-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test: Integrate Git Hub Action Results into Tide #11262
test: Integrate Git Hub Action Results into Tide #11262
Conversation
Hi @VaniHaripriya. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
b63e395
to
1cf98e2
Compare
.github/workflows/ci-checks.yml
Outdated
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
steps: | ||
- name: Remove 'ci-passed' Label on PR Synchronize |
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.
For clarity I would rename this to "Reset ci-passed label status on PR Syncronization" - as it stands it suggests the label should no longer exist after a sync event (even though the very next check is attempting to re-add it)
.github/workflows/ci-checks.yml
Outdated
pull-requests: write | ||
|
||
jobs: | ||
remove_ci_passed_label: |
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.
similar to below, perhaps we should rename this to say reset
instead of remove
64c92ac
to
d462580
Compare
.github/workflows/ci-checks.yml
Outdated
- name: Add 'ci-passed' label | ||
if: success() | ||
run: | | ||
gh pr edit ${{ github.event.pull_request.number }} --add-label "ci-passed" --repo $GITHUB_REPOSITORY |
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 seems like the gh pr edit --add-label
command requires that the labels you are adding already exist in your repository. It cannot create new labels on the fly; it can only assign existing labels to pull requests. From the GitHub CLI manual for gh pr edit:
--add-label stringArray
Add labels by name
This makes me think that the command expects the label names provided to match existing labels in the repository.
.github/workflows/ci-checks.yml
Outdated
- name: Reset ci-passed label status on PR Syncronization | ||
run: | | ||
echo "Resetting 'ci-passed' label as new changes have been pushed." | ||
gh pr edit ${{ github.event.pull_request.number }} --remove-label "ci-passed" --repo $GITHUB_REPOSITORY || echo "Label not present" |
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.
same as https://github.com/kubeflow/pipelines/pull/11262/files#r1783673378
I believe we'd need to add a step to create the label for this repo: https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels
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.
/rerun-all |
/ok-to-test (not sure if that label is needed for /rerun-all to work) |
/rerun-all |
.github/workflows/ci-checks.yml
Outdated
- name: Check if all CI checks passed | ||
uses: wechuli/allcheckspassed@0b68b3b7d92e595bcbdea0c860d05605720cf479 | ||
with: | ||
GITHUB_TOKEN: ${{ env.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.
I'm not sure this is needed (There is a warning in the GHAction result logs about GITHUB_TOKEN
being an unexpected key).
Documentation suggests this either should be under steps[].env
, or more likely this is already covered by the job env
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.
Good catch @gmfrasca will update accordingly..
4d556b1
to
dc64098
Compare
sounds like the GITHUB_TOKEN provided doesn't have proper permissions to add the label (likely needs |
cb66d69
to
e0859f0
Compare
1c36394
to
79a6893
Compare
the token's permissions are limited when runnin on PRs
we have to be careful here, if we give this workflow the permission to add labels, users will be able to change the workflow and game their way to a merge by adjusting the checks/labels you can use workflow_run event to put a workflow in main , and run the label add functionality there, this should have access to ${{ secrets.GITHUB_TOKEN }} because it's running from main branch |
dc26665
to
62fac1c
Compare
.github/workflows/labeling.yml
Outdated
@@ -0,0 +1,24 @@ | |||
# This workflow adds the 'ci-passed' label to a pull request once the 'CI Check' workflow completes successfully. | |||
|
|||
name: Labeling 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.
The name "Labeling Workflow" and labeling.yml
gives the impression that this workflow is intended for any label.
We should probably rename it (and the file) to something like "Add ci-passed label".
005d8f0
to
76d3187
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
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
Signed-off-by: vmudadla <[email protected]>
6278cfc
to
91328b1
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbelmiro The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Description of your changes:
ci-passed
label once all CI checks have passed.ci-passed
label when the pull request is either synchronized or reopened.PR to update GoogleCloudPlatform/oss-test-infra
GoogleCloudPlatform/oss-test-infra#2392
Checklist: