actions-netlify
actions-netlify copied to clipboard
nwtgck
Feature Request: Add option for providing PR context (useful for `workflow_run`)
To get this action to work with non-collaborator PRs I am following the advice to move job steps that require secrets into a separate workflow_run workflow. I am providing the secrets to deploy, the build directory to publish and other field values that I can, this all works as expected.
The action is no longer able to add a comment, deployment environment status, check suite status (enable-commit-status), etc. enable-commit-comment appears to be working, but it is taking the commit/SHA from my master branch that the workflow_run is running from as it's not aware of my pull request anymore. This adds a deployment comment on a commit that is relevant to the pull request.
I use a different action to manage the PR comments. It has a number option to provide PR number (used if no pull_request event is found):
- uses: marocchino/sticky-pull-request-comment@v2
with:
header: preview
number: ${{ env.PR_NUMBER }}
message: |
[Documentation preview for this PR](${{ steps.preview.outputs.deploy-url }}) is ready! :tada:
Built with commit: ${{ env.PR_HEADSHA }}
By providing the PR number (transferred via artifact), I am able to get my comments updated again.
Personally I don't need this feature, but it might be beneficial to other users of your action?
This action may benefit from a similar pr_number feature? The only feature I am losing with my setup with workflow_run is the deployment status updates on the pull request, but the PR comment fulfills the same purpose.
The deployment options are still valid, the project activity log can be viewed to see that a new deployment is added, and a CLI API query lists the deployment description. The only incorrect part is the commit (my master commit instead of actual PR commit (pull_request.head.sha)), I have that information to provide the action with.
I understand what you (people who may want this) want to do.
However, I think that will make the maintainability low. For now, I will wait for GitHub preparing some way for providing secret securely for forked repo.
? I want to be able to say here is the pull request number, treat the action as if it was running from that PR or that commit.
This would mean that when I run workflow_run event on master branch, it does not have this action think it is for the latest master commit (deployment commit message, check suite status on commit, deployment envionrment commit, etc).
It would allow to add/update the comment on the intended PR etc.
More details
This is not about pull_request event which has limited secrets. I am not sure how it affects this project's maintainability, it is adding an option to provide context. Imagine if you want to use this action via workflow_dispatch event instead for manual trigger of the action by providing inputs. The action itself is unable to get this context correctly, so for it to work properly the workflow would need to provide it to the action inputs.
As stated, an action I use for PR comments supports this and works as intended.
If this is a problem for you to support, that is fine. The action is otherwise excellent and I can just disable the features and replace them with additional actions where needed, but I think most users of your action would use it for pull_request deploy previews and want to keep their workflows and maintenance minimal.
I am happy to provide my workflow(s) for documentation so future users learning about the issue can avoid wasting time, however I'm not sure how useful that is if the feature set of this action would remain limited.
@nwtgck Hello, consider this:
https://github.com/nwtgck/actions-netlify/blob/4ad40dcf68dab431d2fc0ba3a0e01e811c034a43/src/main.ts#L48
- const deployRef = context.payload.pull_request?.head.sha ?? context.sha
+ const deployRef = context.payload.pull_request?.head.sha ?? github.event.workflow_run.head_sha ?? context.sha
I'm unsure what the context.* equivalent is, perhaps context.payload.workflow_run.head_sha? I am referencing this event value and it matches the pull_request.head.sha value, or whatever the head sha value is when available from the workflow that triggered workflow_run.
If this is unacceptable, could you please consider another option to disable "Github Deployment Environment"?
Note you also reference commit SHA here:
https://github.com/nwtgck/actions-netlify/blob/4ad40dcf68dab431d2fc0ba3a0e01e811c034a43/src/main.ts#L148
and here:
https://github.com/nwtgck/actions-netlify/blob/4ad40dcf68dab431d2fc0ba3a0e01e811c034a43/src/main.ts#L216-L220
Maybe it would be useful if you extract this into function call, as one is still only using context.sha?
For comment on pull request, your code just checks for number. This one can only be better supported by user providing issue.number to the action inputs. If that is not maintainable for you, then suggesting user alternative action is solution too.
https://github.com/nwtgck/actions-netlify/blob/4ad40dcf68dab431d2fc0ba3a0e01e811c034a43/src/main.ts#L162-L192
Made two PRs:
- README docs for troubleshooting with workarounds added to restore features.
-
Possible
workflow_runcommit fix, if this works only the PR comments is unsupported.
I hope GitHub adds feature like "Approve and Run" with secrets checkbox. The following image is my virtual image.
@polarathene I made a simple solution to preview a pull request from forked repository.
https://user-images.githubusercontent.com/10933561/119133709-2b952000-ba77-11eb-8df8-858568db12cc.mp4
Here is the workflow. It is easy to introduce because the workflow is in a single file and depends only this action and GitHub official one.
# .github/workflows/preview_pr.yml
name: Preview forked PR
on:
workflow_dispatch:
inputs:
pull_request:
description: 'Pull request number'
required: true
jobs:
preview:
runs-on: ubuntu-18.04
steps:
- uses: actions/checkout@v2
with:
# 0 indicates all history
fetch-depth: 0
- name: Merge without push
id: merge
uses: actions/github-script@v4
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
const {execSync} = require('child_process');
function echoExecSync(cmd) { console.log(cmd); execSync(cmd) }
const r = await github.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: ${{ github.event.inputs.pull_request }}
});
echoExecSync(`git config user.email "github-actions[bot]@users.noreply.github.com"`);
echoExecSync(`git config user.name "github-actions[bot]"`);
const baseRef = r.data.base.ref;
echoExecSync(`git checkout ${baseRef}`);
echoExecSync(`git checkout -b preview ${baseRef}`);
echoExecSync(`git pull https://github.com/${r.data.head.repo.full_name}.git ${r.data.head.ref}`);
core.setOutput('sha', r.data.head.sha);
# ... Build here ...
- name: Deploy to Netlify
id: netlify
uses: nwtgck/[email protected]
with:
publish-dir: './public'
github-token: ${{ secrets.GITHUB_TOKEN }}
deploy-message: "Deploy from GitHub Actions"
enable-pull-request-comment: false
enable-commit-comment: false
enable-commit-status: false
env:
NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }}
NETLIFY_SITE_ID: ${{ secrets.NETLIFY_SITE_ID }}
timeout-minutes: 1
- name: Create commit status
uses: actions/github-script@v4
with:
github-token: ${{secrets.GITHUB_TOKEN}}
script: |
await github.repos.createCommitStatus({
owner: context.repo.owner,
repo: context.repo.repo,
context: 'Netlify',
description: 'preview',
state: 'success',
sha: "${{ steps.merge.outputs.sha }}",
target_url: "${{ steps.netlify.outputs.deploy-url }}"
})
actual PR: https://github.com/nwtgck/use-actions-netlify/pull/4
I made a simple solution to preview a pull request from forked repository.
That looks like manual trigger only? I much prefer the split workflow with pull_request and workflow_run events like demonstrated here. I have an updated version of that which has since been merged to an existing project I contribute to.
Especially when I am contributing to other projects, all the extra noise from actions/github-script is going to come across as a maintenance burden.
This action should be able to support workflow_run or workflow_dispatch just as well when provided some additional context, I'll try find some time to learn about writing Github Actions and contribute a PR here.
# ... Build here ...
Additionally this is bad. The whole reason you're doing this is to workaround the restrictions put in place for security reasons... Don't build pull requests from untrusted contributions when they have access to secrets.
As it requires a manual trigger, this does allow for some review to trust the PR contents more, but the blog article explains that is not always foolproof if a PR contains dependency updates/changes that appear harmless at a glance. If no secrets are required for a build, it's better to avoid having secrets available at build time.
Thanks.
If no secrets are required for a build, it's better to avoid having secrets available at build time.
I heavy agree with this idea, and I understand now why you splitted build workflow and preview. I always review and regenerate generation files and merge in some projects.
Especially when I am contributing to other projects, all the extra noise from actions/github-script is going to come across as a maintenance burden.
I agree with the maintenance burden. However, I prefer to use official actions for security reason as much as possible.
However, I prefer to use official actions for security reason as much as possible.
I understand and support that. The main problem at the moment is Github has been advising to split workflow into two and use artifacts but for well over 1 year has not added support for artifact download across workflows. Only supported via the official API with copy/paste snippet to use with actions/github-script or unofficial action.
The unofficial action is more maintainable but poses it's risks, hopefully Github will better address this (or add support to better manage secrets access within a single workfow). For my own usage, I don't mind copy/paste snippets, but when contributing to others to offload maintenance to that projects maintainer, they tend to prefer sharing / using an action that other community projects in open-source use, rather than understand/trust the snippet (or maintenance concerns that come along with that).
Regarding this action though, PR comment, commit status and comment, etc should all be able to work as long as they have the correct context (pull request number and commit SHA).
Presently I substitute this functionality with other actions, but this action continues to create deployment environment status that doesn't reference the PR/commit, instead it seems to reference the latest commit on master branch.
The workflows involved are:
-
pull_requestevent to prepare the build without secrets and upload artifact. -
workflow_runevent to receive artifact, restore context via ENV and pass to actions that need to know PR number or commit SHA.
If you disagree with supporting these inputs that should be documented, and ideally a way to disable the Deployment Environment feature so that it can also be managed correctly outside of the action?
Thanks a lot.
pass to actions that need to know PR number or commit SHA.
I think SHA only is fine without PR number. This decision may be similar to whether preferring thin thing or battery-included things. I should not make this action fat. If it is fat, I this the maintainability gets low in the future.
Disabling Deployment Environment feature is good no matter how we solve this issue.
I think SHA only is fine without PR number.
I am referencing the source of this action for feature that expects to know PR number to comment on:
https://github.com/nwtgck/actions-netlify/blob/8072b2e91159aeb7411f26e6096f4f95b007c089/src/main.ts#L182-L185
Everything else just needs SHA provided. I would look at my PR for supporting workflow_run via event to get correct PR SHA, the shared function is useful for being DRY.
It may be better/simpler to take a new input for SHA if provided to the action by the workflow config, which should support other types of workflow like your workflow_dispatch event as well.
To support features you have like PR comment in such workflows, I think this would benefit with an input as well.
I don't see that as making the action "fat", just accepting two new inputs when required for the action to work, those values are used same way, no extra logic required to support beyond two new optional inputs.
Disabling Deployment Environment feature is good no matter how we solve this issue.
That'd be good! :)
no extra logic required to support beyond two new optional inputs.
Yes in current implementation. My concern is the future implementation. The action should keep simulating run on a given pull request. This means we may need extra inputs to support the feature.
As another idea, the following a new action might be helpful. uses: switch-to-pr-context changes the whole context and switch to pull request context.
- uses: switch-to-pr-context # new action (not realized yet)
with:
issue_number: 32 # you can use ${{ }} to embed
- uses: nwtgck/[email protected]
...
- uses: restore-context
The context is made by some environment variables: https://github.com/actions/toolkit/blob/a1b068ec31a042ff1e10a522d8fdf0b8869d53ca/packages/github/src/context.ts#L28-L53. This may break workflows, but general solution which can be used in other actions.
you have like PR comment in such workflows
Yes.
In the future plan, the comment functionality will be disabled by default in the future major release. Honestly saying, I had not noticed that the commit-status feature is provided by GitHub API until I saw https://github.com/nwtgck/actions-netlify/issues/262. If I knew it at the begging of creating this action, I would not support the comment feature because it makes the action "fat", which actually happens now.
I know people disabling the comment feature and I am one of them. That is annoying because the comment mails me (actually I filtered).
This means we may need extra inputs to support the feature.
Can you think of any additional inputs that may be relevant?
You don't want this action to be "fat", but the core functionality/features should work properly with best practices (eg workflow_run which Github officially encourages for pull requests that need secrets).
Specifying the commit SHA for the action to use seems like a valid input to support. If it's not something you agree with that is fine, someone else can have netlify action to support that, but it should be clear if your action will be compatible with events like workflow_run.
As another idea, the following a new action might be helpful.
I don't see how that is helpful? All that matters with the comment feature being removed is that the other features have the correct commit SHA context. This is all the same commit SHA is it not? Only difference is where that information is taken from (eg push and pull_request/pull_request_target events).
In the PR I replaced the 3 places that information is accessed from to all use single function to get that information instead. All it needs to do is alternatively support a new optional input that allows specifying what SHA to use. It is also useful if something goes wrong to re-run via workflow_dispatch on previous commit, or for manual trigger or automated (eg split or recurring workflow via workflow_run).
Your action does not need to worry about all different scenarios and how to get that commit SHA, just supporting the push and pull_request ones as it presently does, and for anything else the user supplies the commit SHA if they need to. I don't think workflow context should be messed with.
In the future plan, the comment functionality will be disabled by default in the future major release.
That's fine by me. I needed a more flexible comment action so disabled this one and used marocchino/sticky-pull-request-comment, your README (and next major release announcement) could mention this for existing users of the feature to migrate to (as well as any new users that want to use the action and have the PR comment). It supports the PR number as input, so works well.
Honestly saying, I had not noticed that the commit-status feature is provided by GitHub API until I saw #262.
As I was using the comment feature, this was not that important to me. But when splitting the workflow, the 2nd part would not have any check/commit-status (probably because of invalid commit SHA).
Originally in single workflow I already had similar (minus the URL) to know if everything was successful, but with the split workflow I would prefer this feature instead of using a commit-status action twice (for pending prior to deploy, and success/failure afterwards).