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

vincepnguyen · 2021-08-25T18:09:08Z

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.

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)

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

kate-goldenring · 2021-08-25T18:38:54Z

We should update our documentation to reflect that we are now using labels. A PR should go into the akri-docs repo and merge right after this one. @vincepnguyen would you be interested in updating the docs?

.github/workflows/build-opencv-base-container.yml

kate-goldenring · 2021-08-25T18:42:59Z

Can you also update the [PR template(https://raw.githubusercontent.com/deislabs/akri/main/.github/pull_request_template.md) to specify using a label instead. I linked it in raw mode so you can see the hidden MD comment that needs to be updated

romoh · 2021-08-25T20:21:03Z

.github/workflows/build-opencv-base-container.yml

@@ -10,6 +10,7 @@ on:
    - build/intermediate-containers.mk
    - Makefile
  pull_request:
+    types: [assigned, opened, synchronize, reopened, labeled, unlabeled]


curious - what is synchronize?

hey Roaa, synchronize is when a commit is pushed to the PR. I found some more details here https://frontside.com/blog/2020-05-26-github-actions-pull_request/

Why do we want the workflow to run whenever someone is assigned to the PR?

Good point! I removed the assigned.

romoh

I think we should be explicit about what the labels mean, so that any contributor can easily interpret them.
"allow intermediate build" and "ignore intermediate build" are very vague. I don't expect others to understand what they mean until they look at the makefile.
How about "use-docker-cache" or "no-docker-cache"?

Is there a way we can only have one label? Default can be docker cache is used, anyone sending a PR touching those special files will re-build the images unless the user explicitly add "no-docker-cache". We don't expect those files to change frequently, right?

[SAME VERSION] Workflow to mark inactive issues/PRs as stale and eventually close them Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

bfjelds · 2021-08-27T18:33:20Z

How about "use-docker-cache" or "no-docker-cache"?

i'm not sure that these are quite right for me ... the flag controls whether this pr actually builds the intermediate containers (crossbuild, opencv), not whether we use caches. for example, the rust containers (controller, agent, etc) will always use the docker cache and these flags don't impact that.

Is there a way we can only have one label? Default can be docker cache is used, anyone sending a PR touching those special files will re-build the images unless the user explicitly add "no-docker-cache". We don't expect those files to change frequently, right?

we can certainly do what you suggest. i prefer 2 flags and people making an assertive choice because with your suggestion, it is easier for us to burn cpu cycles unintentionally. that probably isn't a huge deal as long as the cpu cycles are unlimited and free, and no one is in a hurry to merge a PR that touches Makefile.

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

kate-goldenring · 2021-11-19T23:46:52Z

@vincepnguyen looks like you've been cranking out a lot of changes. Give us a bump when this is ready for another look through.

vincepnguyen · 2021-11-19T23:47:35Z

/add-same-version-label

vincepnguyen · 2021-11-19T23:48:57Z

/add-build-dependency-containers-label

vincepnguyen · 2021-11-20T00:01:14Z

@bfjelds @kate-goldenring @romoh hey everyone, this PR is ready for review :)

kate-goldenring · 2021-11-22T14:23:19Z

@vincepnguyen can you sign your commits so as to make the DCO pass? More information here.

kate-goldenring · 2021-11-22T14:30:11Z

.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 a 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".'


Just curious, why did you choose to have a preceding backslash in "/add-build-dependency-containers-label"

nit: remove "a" in "should run a build dependency containers"

also clarification: they can also use the normal GitHub UI to add the label right?

can everyone use our labels through the ui or just our admins?

.github/workflows/build-rust-crossbuild-container.yml

kate-goldenring · 2021-11-22T14:35:09Z

.github/workflows/build-rust-crossbuild-container.yml

    - if: >-
        startsWith(github.event_name, 'pull_request') &&
-        !contains(github.event.pull_request.title, '[SAME VERSION]') &&
-        !contains(github.event.commits[0].message, '[SAME VERSION]')
+        contains(github.event.pull_request.labels.*.name, 'same version') == false


nit: maybe keep !contains(...) instead of == false

.github/workflows/build-rust-crossbuild-container.yml

kate-goldenring · 2021-11-22T14:39:58Z

.github/workflows/build-opencv-base-container.yml

+
+  add-label-missing-comment:
+    runs-on: ubuntu-latest
+    if: github.event_name == 'pull_request' && (github.event.action == 'opened' || github.event.action == 'unlabeled') && contains(github.event.pull_request.labels.*.name, 'build dependency containers') == false


What is this github.event.action == 'unlabeled' trying to capture? What if there is some other label like bug or documentation on the PR? Seems like just the opened expression is sufficient.

kate-goldenring · 2021-11-22T14:42:32Z

This is looking great

@bfjelds @kate-goldenring @romoh hey everyone, this PR is ready for review :)

@vincepnguyen this is looking great! Thanks for adding to auto labeling feature! Just had some nits and clarifying questions

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com> Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen · 2021-11-23T00:49:47Z

Some command to rebase and sign off messed up previous commits so I am going to abandon this PR and create a fresh new one and resolve your comments there.

kate-goldenring and others added 9 commits August 12, 2021 12:28

Remove Kubernetes components from ONVIF broker (#356)

07e5bd6

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

Move documentation out of Akri's main repository [SAME VERSION] (#360)

0932855

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

add stale issues marker workflow

fece4e9

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

update based on feedback

c895c9e

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

update

2c6622e

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

not

3dd7cb3

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

update messages

2fdc7bc

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

Update Akri Kubernetes and Runtime Dependencies (#361)

d811a35

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

dry-run

7ead17d

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen requested review from bfjelds, Britel, jiria, kate-goldenring and romoh as code owners August 25, 2021 18:09

kate-goldenring changed the title ~~User/vinguyen/buildbylabel~~ Execute workloads based on labels instead of flags in PR titles Aug 25, 2021

kate-goldenring added ignore intermediate builds same version allow intermediate builds and removed same version ignore intermediate builds allow intermediate builds labels Aug 25, 2021

kate-goldenring reviewed Aug 25, 2021

View reviewed changes

.github/workflows/build-opencv-base-container.yml Outdated Show resolved Hide resolved

romoh reviewed Aug 25, 2021

View reviewed changes

romoh requested changes Aug 25, 2021

View reviewed changes

Merge pull request #363 from romoh/main

50f48a2

[SAME VERSION] Workflow to mark inactive issues/PRs as stale and eventually close them Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen changed the title ~~Execute workloads based on labels instead of flags in PR titles-~~ Execute workloads based on labels instead of flags in PR titles Nov 19, 2021

vincepnguyen added 7 commits November 19, 2021 14:27

Execute workloads based on labels instead of flags in PR titles

b9547b2

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

addressed comments

995d805

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

auto comment on PR and add label by comment

35c77bc

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

use later version of github-scripts

8f8274b

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

fix issues with add label and add comments on PR

94a3785

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

trigger build flag check on comment edit event

4611d33

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

only add comment when the PR is opened/unlabeled PR

f8d5ddf

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen force-pushed the user/vinguyen/buildbylabel branch from c32f7e2 to f8d5ddf Compare November 19, 2021 22:30

update contributing docs

0914853

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen force-pushed the user/vinguyen/buildbylabel branch from e0b2ed9 to 0a76c0f Compare November 19, 2021 23:46

vincepnguyen added the same version label Nov 19, 2021

vincepnguyen requested review from bfjelds and kate-goldenring November 19, 2021 23:56

kate-goldenring reviewed Nov 22, 2021

View reviewed changes

.github/workflows/build-rust-crossbuild-container.yml Show resolved Hide resolved

kate-goldenring reviewed Nov 22, 2021

View reviewed changes

.github/workflows/build-rust-crossbuild-container.yml Show resolved Hide resolved

kate-goldenring reviewed Nov 22, 2021

View reviewed changes

Merge branch 'main' into user/vinguyen/buildbylabel

48c5ed1

Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com> Signed-off-by: vincepnguyen <70007233+vincepnguyen@users.noreply.github.com>

vincepnguyen force-pushed the user/vinguyen/buildbylabel branch 2 times, most recently from d30b770 to 48c5ed1 Compare November 23, 2021 00:46

vincepnguyen closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vincepnguyen commented Aug 25, 2021

kate-goldenring commented Aug 25, 2021

kate-goldenring commented Aug 25, 2021

romoh Aug 25, 2021

vincepnguyen Nov 19, 2021

kate-goldenring Nov 22, 2021

vincepnguyen Nov 23, 2021

romoh left a comment •

edited

bfjelds commented Aug 27, 2021 •

edited

kate-goldenring commented Nov 19, 2021

vincepnguyen commented Nov 19, 2021

vincepnguyen commented Nov 19, 2021

vincepnguyen commented Nov 20, 2021

kate-goldenring commented Nov 22, 2021

kate-goldenring Nov 22, 2021

kate-goldenring Nov 22, 2021

kate-goldenring Nov 22, 2021

bfjelds Nov 22, 2021

kate-goldenring Nov 22, 2021

kate-goldenring Nov 22, 2021

kate-goldenring commented Nov 22, 2021 •

edited

vincepnguyen commented Nov 23, 2021

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

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

Conversation

vincepnguyen commented Aug 25, 2021

kate-goldenring commented Aug 25, 2021

kate-goldenring commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romoh left a comment • edited

Choose a reason for hiding this comment

bfjelds commented Aug 27, 2021 • edited

kate-goldenring commented Nov 19, 2021

vincepnguyen commented Nov 19, 2021

vincepnguyen commented Nov 19, 2021

vincepnguyen commented Nov 20, 2021

kate-goldenring commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kate-goldenring commented Nov 22, 2021 • edited

vincepnguyen commented Nov 23, 2021

romoh left a comment •

edited

bfjelds commented Aug 27, 2021 •

edited

kate-goldenring commented Nov 22, 2021 •

edited