-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature Request: Add option for providing PR context (useful for workflow_run
)
#545
Comments
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. |
This comment has been minimized.
This comment has been minimized.
@nwtgck Hello, consider this: Line 48 in 4ad40dc
- 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 If this is unacceptable, could you please consider another option to disable "Github Deployment Environment"? Note you also reference commit SHA here: Line 148 in 4ad40dc
and here: Lines 216 to 220 in 4ad40dc
Maybe it would be useful if you extract this into function call, as one is still only using |
For comment on pull request, your code just checks for number. This one can only be better supported by user providing Lines 162 to 192 in 4ad40dc
|
Made two PRs:
|
@polarathene actions-netlify-preview-forked-pr.mp4Here 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: nwtgck/use-actions-netlify#4 |
That looks like manual trigger only? I much prefer the split workflow with Especially when I am contributing to other projects, all the extra noise from This action should be able to support
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.
I heavy agree with this idea, and I understand now why you splitted build workflow and preview. |
I agree with the maintenance burden. 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 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:
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? UPDATE: For reference, the workflow links reference the |
Thanks a lot.
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 am referencing the source of this action for feature that expects to know PR number to comment on: Lines 182 to 185 in 8072b2e
Everything else just needs SHA provided. I would look at my PR for supporting 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 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.
That'd be good! :) |
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 # 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. |
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 #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). |
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 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
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 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 Your action does not need to worry about all different scenarios and how to get that commit SHA, just supporting the
That's fine by me. I needed a more flexible comment action so disabled this one and used
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). |
Closing due age of issue and lack of activity. |
I’m hitting this problem right now, where after moving the deploy to the |
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 theworkflow_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):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 withworkflow_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.The text was updated successfully, but these errors were encountered: