-
Notifications
You must be signed in to change notification settings - Fork 45
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
Added envoy-sync-receive workflow #247
Conversation
b1b0499
to
ce63a15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good @tedjpoole, but will need a bit of setup with the repo/s and/or bot/s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, LGTM, just had a few nits.
I have zero idea how the workflow works, but I trust you :)
Hint: You may run shellcheck
against this script, and, if you use vscode, you can install the shellcheck
extension.
# - adds a comment on the associated issue to describe the merge fail | ||
# | ||
|
||
set -xeuo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe -x
is too much? We're doing a few echo
s, isn't that enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the -x
on the assumption that (1) there probably will be some bugs to fix, and (2) the -x
will make it easier to fix those bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pay double attention to not leak any secret/token.
ci/envoy-sync-receive.sh
Outdated
DST_NEW_BRANCH_NAME="auto-merge-$(echo "${SRC_BRANCH_NAME}" | tr /. -)" | ||
|
||
# Set the default remote for the gh command | ||
gh repo set-default "${DST_REPO_ORG}/${DST_REPO_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing gh
only needs the GITHUB_TOKEN
variable to be set in order to work, right? (which is defined in the job yaml file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, actually good point, if we are using the GITHUB_TOKEN
(and setting relevant creds) we dont need the appauth stuff which essentially provides write creds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the gh
command gets its auth token from the GITHUB_TOKEN
(or GH_TOKEN
) environment variable (see docs).
However, it seems like the default secrets.GITHUB_TOKEN
is not sufficient for our needs. It is limited to just the repository scope and cannot access projects or create/update issues etc.
@phlax would it be appropriate to pass the ${{ steps.appauth.outputs.token }}
token from the upstream envoy-sync.yml
workflow, as an input to the downstream envoy-sync-receive.yaml
workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is limited to just the repository scope and cannot access projects or create/update issues etc
i think you need
permissions:
issues: write
pull-requests: write
creating/updating issues/prs should work with that
re appauth - i think we dont need it at all if we are using the GITHUB_TOKEN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that is stil repo-scoped tho)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Adding the issues: write
permission enabled the gh issue create
and gh issue comment
commands to work.
However, despite adding the pull-requests: write
permission, the gh pr create
command still fails to create a pull request:
++ gh pr create --head auto-merge-main --base main --title 'auto-merge envoyproxy/envoy[main] into tedjpoole/test-downstream[main]' --body 'Generated by envoy-sync-receive.sh'
pull request create failed: GraphQL: GitHub Actions is not permitted to create or approve pull requests (createPullRequest)
Also, while testing locally I encountered this error from the git push
command. It seems to be caused by the fact that the merge had brought in some upstream changes to workflow files under the .github/workflows/
directory, and when pushing any changes to workflow files, github requires the actor to have workflows: write
permission, which cannot be given to the secrets.GITHUB_TOKEN
.
+ git push --force origin HEAD:auto-merge-main
To https://github.com/tedjpoole/test-downstream
! [remote rejected] HEAD -> auto-merge-main (refusing to allow a GitHub App to create or update workflow `.github/workflows/_cache.yml` without `workflows` permission)
error: failed to push some refs to 'https://github.com/tedjpoole/test-downstream'
Therefore, it does seem that the secrets.GITHUB_TOKEN
cannot be given some of the permissions that we need in the script.
I know I need to use a different token which has the additional permissions, but I'm not really sure of the best way to achieve that.
If I reinstate this step:
- id: appauth
uses: envoyproxy/toolshed/gh-actions/appauth@actions-v0
with:
key: ${{ secrets.ENVOY_CI_UPDATE_BOT_KEY }}
app_id: ${{ secrets.ENVOY_CI_UPDATE_APP_ID }}
then I think I can use ${{ steps.appauth.outputs.value }}
in place of secrets.GITHUB_TOKEN
, but I'm not sure where I need to create those two secrets, or even if a am able to, nor where I can configure the permissions that are required by the script.
Any advice or pointers welcome thanks
5c8be00
to
1cdaa1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @tedjpoole
nit re appauth
- better to use token
- see here:
1cdaa1d
to
941ff44
Compare
941ff44
to
c7222e6
Compare
Signed-off-by: Ted Poole <tpoole@redhat.com>
c7222e6
to
2b43b52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workflow changes lgtm, thanks @tedjpoole
ill update the wf trigger bot now to give creds in this repo
Tested with a GitHub App in my own account, and confirmed that it needs the following permissions:
Note, the |
Added
envoy-sync-receive.yaml
workflow. This is designed to be invoked from theenvoy-sync.yaml
workflow from the upstreamenvoyproxy/envoy
repository (see envoyproxy/envoy#34319).It performs a merge from the specified branch in upstream
envoyproxy/envoy
into the same named branch inenvoyproxy/envoy-openssl
e.g.For each distinct branch name, the script manages at most 1 associated pull request and/or issue, depending on the success of the merge.
If the merge from upstream succeeds, it creates or updates the associated pull request, and closes the associated issue (if there is one).
If the merge from upstream fails, it leaves the associated pull request untouched (if there is one), and creates or updates the associated issue.