test: skipping envoy coverage CI for E-M changes #24118

alyssawilk · 2022-11-21T20:41:37Z

If this works, will apply to most Azure workflows.

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

co-authored-by phlax

repokitteh-read-only · 2022-11-21T20:41:42Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24118 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-11-21T20:47:53Z

cc @phlax I'd like your thoughts on this

I like what Enovy Mobile does to avoid spurious CI work:
https://github.com/envoyproxy/envoy-mobile/blob/main/.github/workflows/android_tests.yml#L23
but I'm not sure of the AZP way to do if, and I thought tweaking do_ci would be not much hackier but want to get your thoughts before I put more time into it.

If SGTY I'll extend to skipping all tests for mobile (and if you'd like, docs)-only changes

phlax · 2022-11-21T21:06:13Z

my ideal is that we can just run bazel test //... and it runs only and all things that should be run

in reality, we are quite far off from that so i guess git diff and bail hacks will have to do for now (there are a few problems with this pattern but its the easiest way i think)

what we could probably do is run some early ci that puts vars into the azp namespace - ie ENVOY_HAS_CHANGES , MOBILE_HAS_CHANGES and then we can get azp to skip specific parts of the CI altogether (ie no git checkouts etc)

phlax · 2022-11-21T21:15:23Z

canonical example for setting vars from inside tasks/steps

variables:
- name: one
  value: initialValue 

steps:
  - script: |
      echo ${{ variables.one }} # outputs initialValue
      echo $(one)
    displayName: First variable pass
  - bash: echo "##vso[task.setvariable variable=one]secondValue"
    displayName: Set new variable value
  - script: |
      echo ${{ variables.one }} # outputs initialValue
      echo $(one) # outputs secondValue
    displayName: Second variable pass

from https://learn.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-11-22T19:13:09Z

cc @phlax so more like this?
I'm less confident this works - not sure how to test other than either submitting and trying a mobile only change, or adding some extra excludes for a test run WDYT?

.azure-pipelines/pipelines.yml

Co-authored-by: phlax <phlax@users.noreply.github.com> Signed-off-by: alyssawilk <alyssar@google.com>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

phlax · 2022-11-24T13:52:12Z

running some tests in #24204

phlax

i ran some tests (with correct bools - apologies for incorrect advice previously) and seems to work - as you can also see in this PR it seems to detect that subsequent stages depend upon change_detection

phlax · 2022-11-24T14:00:24Z

.azure-pipelines/pipelines.yml

@@ -23,8 +23,23 @@ variables:
 - name: isStableBranch
  # A release branch can be either `main` or a `release/v1.x` stable branch
  value: $[or(eq(variables['isMain'], 'true'), eq(variables['isReleaseBranch'], 'true'))]
+- name: nonMobileFilesChanged
+  value: True


Suggested change

value: True

value: true

phlax · 2022-11-24T14:00:37Z

.azure-pipelines/pipelines.yml

+    steps:
+    - bash: |
+        git diff --name-only origin/main | grep -qe common/ \
+          || echo "##vso[task.setvariable variable=nonMobileFilesChanged]False"


Suggested change

|| echo "##vso[task.setvariable variable=nonMobileFilesChanged]False"

|| echo "##vso[task.setvariable variable=nonMobileFilesChanged]false"

phlax · 2022-11-24T14:01:19Z

.azure-pipelines/pipelines.yml

@@ -339,6 +354,7 @@ stages:
        ciTarget: $(CI_TARGET)

  - job: coverage
+    condition: ne(variables['nonMobileFilesChanged'], '')


Suggested change

condition: ne(variables['nonMobileFilesChanged'], '')

condition: eq(variables['nonMobileFilesChanged'], true)

phlax · 2022-11-24T14:03:33Z

.azure-pipelines/pipelines.yml

+      vmImage: "ubuntu-20.04"
+    steps:
+    - bash: |
+        git diff --name-only origin/main | grep -qe common/ \


Suggested change

git diff --name-only origin/main | grep -qe common/ \

git diff origin/main..HEAD -- . :^mobile

i think this is the correct diff for non-mobile changes (EDIT: its not correct) between HEAD and main - its still problematic tho as comparing to main is incorrect on other branches - we want to compare to current target branch

if we really only want to run coverage when common/ changed then i think git diff origin/main..HEAD -- common/ - but then i think we rename the var

thinking about this we could skip coverage for a whole bunch of changes

a much better (working) suggestion for the git command to the one above:

git diff-index --quiet origin/$(System.PullRequest.TargetBranch) :^mobile \ || ...

this is still problematic as it only works on pull requests (comparing to main on main will always be empty so it would switch off postsubmit altogether if we only test on diffs to main/etc)

for main/release branches we probably want to compare to the last commit (or commit of last landed pr in case of release branches)

phlax · 2022-11-24T16:31:39Z

arggh

testing this further and it would seem doing echo "##vso[task.setvariable variable=nonMobileFilesChanged]false only sets the value in the current stage and not in the task as it would suggest

reading the docs more thoroughly seems to confirm this altho they also contradict what it does do

this pattern can still be useful if for example we wanted to prevent some or all of the check jobs from running - we can still just check what has changed once and skip subsequent steps conditionally, we just cant do it across the entire pipeline

phlax · 2022-11-24T17:06:51Z

hmm, actually does seem possible (see eg https://praveenkumarsreeram.com/2022/05/03/azure-devops-tips-and-tricks-5-how-to-pass-values-between-tasks-in-a-pipeline-using-task-setvariable-logging-command/)

the variable needs to be set with isoutput=true and the subsequent references to it need to prefix with the "TaskName"

ill test this further

phlax · 2022-11-28T13:56:04Z

this turned out to be a bit more complex than i had realized and the azp interface is not the easiest to work with

for these reasons ive created a PR to just add the change detection (with some docstrings on usage) here #24221

alyssawilk force-pushed the skip_where_possible branch from af228f7 to 6344b7d Compare November 21, 2022 20:42

test: skipping envoy CI for E-M changes

939900f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the skip_where_possible branch from 6344b7d to 939900f Compare November 21, 2022 20:47

trying azure style

ca15f9b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the skip_where_possible branch from 32af813 to ca15f9b Compare November 22, 2022 17:47

alyssawilk marked this pull request as ready for review November 22, 2022 20:56

phlax reviewed Nov 23, 2022

View reviewed changes

.azure-pipelines/pipelines.yml Outdated Show resolved Hide resolved

.azure-pipelines/pipelines.yml Show resolved Hide resolved

jmarantz assigned phlax Nov 23, 2022

alyssawilk and others added 3 commits November 23, 2022 11:32

Update .azure-pipelines/pipelines.yml

e5a0ffe

Co-authored-by: phlax <phlax@users.noreply.github.com> Signed-off-by: alyssawilk <alyssar@google.com>

Update .azure-pipelines/pipelines.yml

e3c08d5

Co-authored-by: phlax <phlax@users.noreply.github.com> Signed-off-by: alyssawilk <alyssar@google.com>

Merge branch 'main' into skip_where_possible

8a02d45

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk changed the title ~~test: skipping envoy CI for E-M changes~~ test: skipping envoy coverage CI for E-M changes Nov 23, 2022

phlax reviewed Nov 24, 2022

View reviewed changes

alyssawilk closed this Nov 28, 2022

alyssawilk deleted the skip_where_possible branch April 5, 2023 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skipping envoy coverage CI for E-M changes #24118

test: skipping envoy coverage CI for E-M changes #24118

alyssawilk commented Nov 21, 2022 •

edited

Loading

repokitteh-read-only bot commented Nov 21, 2022

alyssawilk commented Nov 21, 2022

phlax commented Nov 21, 2022

phlax commented Nov 21, 2022

alyssawilk commented Nov 22, 2022

phlax commented Nov 24, 2022

phlax left a comment

phlax Nov 24, 2022

phlax Nov 24, 2022

phlax Nov 24, 2022

phlax Nov 24, 2022 •

edited

Loading

phlax Nov 24, 2022

phlax Nov 24, 2022

phlax Nov 24, 2022 •

edited

Loading

phlax Nov 24, 2022 •

edited

Loading

phlax commented Nov 24, 2022 •

edited

Loading

phlax commented Nov 24, 2022

phlax commented Nov 28, 2022

	\|\| echo "##vso[task.setvariable variable=nonMobileFilesChanged]False"
	\|\| echo "##vso[task.setvariable variable=nonMobileFilesChanged]false"

	condition: ne(variables['nonMobileFilesChanged'], '')
	condition: eq(variables['nonMobileFilesChanged'], true)

	git diff --name-only origin/main \| grep -qe common/ \
	git diff origin/main..HEAD -- . :^mobile

test: skipping envoy coverage CI for E-M changes #24118

test: skipping envoy coverage CI for E-M changes #24118

Conversation

alyssawilk commented Nov 21, 2022 • edited Loading

repokitteh-read-only bot commented Nov 21, 2022

alyssawilk commented Nov 21, 2022

phlax commented Nov 21, 2022

phlax commented Nov 21, 2022

alyssawilk commented Nov 22, 2022

phlax commented Nov 24, 2022

phlax left a comment

Choose a reason for hiding this comment

phlax Nov 24, 2022

Choose a reason for hiding this comment

phlax Nov 24, 2022

Choose a reason for hiding this comment

phlax Nov 24, 2022

Choose a reason for hiding this comment

phlax Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

phlax Nov 24, 2022

Choose a reason for hiding this comment

phlax Nov 24, 2022

Choose a reason for hiding this comment

phlax Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

phlax Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

phlax commented Nov 24, 2022 • edited Loading

phlax commented Nov 24, 2022

phlax commented Nov 28, 2022

alyssawilk commented Nov 21, 2022 •

edited

Loading

phlax Nov 24, 2022 •

edited

Loading

phlax Nov 24, 2022 •

edited

Loading

phlax Nov 24, 2022 •

edited

Loading

phlax commented Nov 24, 2022 •

edited

Loading