Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Execute workloads based on labels instead of flags in PR titles #426

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Execute workloads based on labels instead of flags in PR titles #426

merged 1 commit into from
Nov 30, 2021

Conversation

vincepnguyen
Copy link
Collaborator

@vincepnguyen vincepnguyen commented Nov 23, 2021 •
edited
Loading

Signed-off-by: vincepnguyen [email protected]

What this PR does / why we need it:
Currently, certain workflows are being skipped/executed based on flags set in PR titles. For example, adding [SAME VERSION] to a PR title prevents the version check workflow from running. However, after adding a flag to a title, checks cannot simply be re-run, since they refer to the state of the PR of the last commit. Resultingly, another commit must be pushed to the PR in order for checks to properly register the PR title change (and therefore flag). Running workflows based on labels instead of flags may remove this issue of pushing new commits. This change enables running build/version check based on labels.

The associated PR with documentation in akri-docs is: project-akri/akri-docs#16

Special notes for your reviewer:

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • version has been updated appropriately (./version.sh)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

@github-actions
Copy link
Contributor

Hi there, this change should run build dependency containers, but running it will take hours. Do you want to run it? If so, please add label "build dependency containers" by commenting "/add-build-dependency-containers-label".

1 similar comment
@github-actions
Copy link
Contributor

Hi there, this change should run build dependency containers, but running it will take hours. Do you want to run it? If so, please add label "build dependency containers" by commenting "/add-build-dependency-containers-label".

@vincepnguyen
Copy link
Collaborator Author

/add-same-version-label

vincepnguyen
vincepnguyen commented Nov 23, 2021
View reviewed changes
.github/workflows/build-opencv-base-container.yml
issue_number: context.payload.pull_request.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'Hi there, this change should run build dependency containers, but running it will take hours. Do you want to run it? If so, please add label "build dependency containers" by commenting "/add-build-dependency-containers-label".'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kate-goldenring

Just curious, why did you choose to have a preceding backslash in "/add-build-dependency-containers-label"
I use preceding "/" to differentiate between generic comments and commands. Thought I don't have a strong reference here. Any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems fine to me

@vincepnguyen
Copy link
Collaborator Author

@kate-goldenring @romoh @bfjelds hey everyone, this is the new PR with your comments addressed. I messed up the old PR trying to sign off older commits. Please help me review this change. Thanks for the feedbacks!

@kate-goldenring kate-goldenring linked an issue Nov 23, 2021 that may be closed by this pull request
Execute workloads based on labels instead of flags in PR titles #334
Closed
@vincepnguyen vincepnguyen mentioned this pull request Nov 23, 2021
Merged
romoh
romoh reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@romoh romoh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, vince. Can we document adding comments (ie: /add-same-version-label) for others to be aware of? what labels are supported? what adding them mean?.. etc
Also this seems like a good thing to mention on slack once the PR is in.

@vincepnguyen
Copy link
Collaborator Author

This looks great, vince. Can we document adding comments (ie: /add-same-version-label) for others to be aware of? what labels are supported? what adding them mean?.. etc Also this seems like a good thing to mention on slack once the PR is in.

Thanks for the feedbacks, Roaa. Yes, the associated PR with documentation in akri-docs is: project-akri/akri-docs#16

@kate-goldenring
Copy link
Contributor

@vincepnguyen Do we still need to create a build dependency containers label?

@vincepnguyen
Copy link
Collaborator Author

@vincepnguyen Do we still need to create a build dependency containers label?

Yes, I created the build dependency containers label.

@kate-goldenring kate-goldenring merged commit 91f757e into main Nov 30, 2021
@kate-goldenring kate-goldenring deleted the vinguyen/buildbylabel branch November 30, 2021 22:59
jiria
jiria reviewed Dec 3, 2021
View reviewed changes
.github/workflows/build-opencv-base-container.yml

env:
AKRI_COMPONENT: opencvsharp-build
MAKEFILE_COMPONENT: opencv-base

jobs:

add-same-version-label-to-pr:
runs-on: ubuntu-latest
if: github.event_name == 'issue_comment' && contains(github.event.comment.body, '/add-same-version-label')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be contains or equals? Or is there no such check?

@kate-goldenring kate-goldenring mentioned this pull request Jan 4, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiria jiria jiria left review comments

@kate-goldenring kate-goldenring kate-goldenring approved these changes

@romoh romoh romoh approved these changes

@bfjelds bfjelds Awaiting requested review from bfjelds bfjelds is a code owner

@Britel Britel Awaiting requested review from Britel Britel is a code owner

Assignees
No one assigned
Labels
same version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Execute workloads based on labels instead of flags in PR titles
4 participants
@vincepnguyen @kate-goldenring @romoh @jiria