Run all relevant tests on all PRs #148

shanth96 · 2024-03-05T20:12:28Z

Fixes https://github.com/Shopify/vitess-project/issues/613

This PR:

Adds two new scripts which are modified versions of the existing scripts that Vitess uses to get the next/previous release for upgrade/downgrade tests.
- I decided to add it as new scripts instead of modifying existing ones so that if the script gets modified upstream, we wouldn't need to do any conflict resolution.
- The script gets the previous release to compare by looking at the most recent -shopify release on the previous major release.
- Similarly, it gets the next release by also searching for the latest -shopify release on the next major release, falling back to an upstream release if a shopify release doesn't exist.
- In the case that we're patching the latest release, it picks main branch.
- The script looks at go/vt/servenv/version.go to get the current version that a PR is making a fix to.
Updates the skip-workflow logic so that we don't skip tests in PRs

🎩 instructions

Go to go/vt/servenv/version.go and update versionName to the value in version.go below.
Run ./tools/get_next_release_shopify.sh and ./tools/get_previous_release_shopify.sh
Compare the output with the respective columns below

Description	version.go	previous_release	next_release
Where previous release doesn’t exist	15.0.3		16.0.7-shopify-1
Where previous release exists but not a shopify next release	16.0.7	15.0.3-shopify-10	17.0.5
Where previous exist but on latest already for next	18.0.0		main

The empty string in previous releases is expected since we don't have any shopify releases that would match. This is okay because whenever we make a patch from now on, we will be guaranteed to have a previous shopify release.

brendar · 2024-03-05T21:00:40Z

Updates the skip-workflow logic

The github workflow files are generated from templates in test/templates, so we should be able to update the skip logic in those templates then run make generate_ci_workflows to regenerate the workflow files.

I updated it so that these tests run anytime a PR is created with a branch with the name pattern vX.Y.Z-shopify-XX-candidate. The idea is that we create a candidate branch that is going to merge into a vX.Y.Z-shopify-XX branch.

Does that mean we don't expect all the tests to be running on this branch since it's called fix-tests?

brendar · 2024-03-05T21:19:13Z

For the previous release logic:
It looks like this workflow produced release-16.7-shopify-1.0 as the previous release, which caused the "Run Upgrade Downgrade Test" part to fail.


Run previous_release_ref=$(./tools/get_previous_release_shopify.sh v16.0.7-shopify-1-candidate refs/pull/148/merge)
  previous_release_ref=$(./tools/get_previous_release_shopify.sh v16.0.7-shopify-1-candidate refs/pull/148/merge)
  echo $previous_release_ref
  echo "previous_release_ref=${previous_release_ref}" >> $GITHUB_OUTPUT
  shell: /usr/bin/bash -e {0}
release-16.7-shopify-1.0

Would we expect that to be v15.0.3-shopify-10?

For the next release logic:
It looks like this workflow produced an empty string, which caused the "Run Upgrade Downgrade Test" part to be skipped because of this logic:

        if [[ "${{needs.get_next_release.outputs.next_release}}" == "" ]]; then
          skip='true'
        fi

Should that have been either release-17.0 (branch) or v17.0.5 (tag) since there is no -shopify-N patch release for v17 yet?

shanth96 · 2024-03-06T16:41:26Z

The github workflow files are generated from templates in test/templates, so we should be able to update the skip logic in those templates then run make generate_ci_workflows to regenerate the workflow files.

Updated to use the templates

Does that mean we don't expect all the tests to be running on this branch since it's called fix-tests?

Yes, it should be skipping tests in this branch. Gonna look at why it ended up trying to run some of the tests

shanth96 · 2024-03-06T18:36:35Z

Should that have been either release-17.0 (branch) or v17.0.5 (tag) since there is no -shopify-N patch release for v17 yet?

I think that makes sense to fallback to an upstream release if a shopify release doesn't exist yet. I've updated the script to do that.

brendar · 2024-03-06T19:52:31Z

Yes, it should be skipping tests in this branch. Gonna look at why it ended up trying to run some of the tests

Would it make sense to never skip tests? It might be nice to have tests running on prototype branches and it would mean we wouldn't need to be careful about always opening PRs from *-candidate-* branches

shanth96 · 2024-03-06T20:57:59Z

Would it make sense to never skip tests? It might be nice to have tests running on prototype branches and it would mean we wouldn't need to be careful about always opening PRs from -candidate- branches

Then we might need a different way of identifying which version a PR is going to merge to. Currently, the version specified in the branch name is used to figure out the versions for the upgrade/downgrade tests. If, for example, we have a fix-foo branch merging into a catch-all branch, we'll need some way of saying that we're merging into a v15 branch so run upgrade tests for v16 and downgrade tests for v14

brendar · 2024-03-06T21:23:25Z

Then we might need a different way of identifying which version a PR is going to merge to.

Presumably the source and target branch of a PR both have the same major version, so maybe we could pull the version from here? https://github.com/Shopify/vitess/blob/main/go/vt/servenv/version.go#L22

shanth96 · 2024-03-07T17:25:14Z

Made some changes:

We now look at servenv file to grab the major version for a given PR
This means we don't need the specific branch naming pattern so we run all workflows at all time. The only time we'll skip a workflow is if it's not a pull request event

pawandubey

Thanks for this. Two comments (apart from the sort usage):

Should we move the _shopify scripts to a /shopify sub directory instead of using the suffix as the name?
Would be great if there were some tophatting instructions, with e.g. what would the expected output of the two scripts be if we were checked out on a given version. For now I have:

HEAD	previous_release	next_release
fix-tests	v15.0.3-shopify-10	v17.0.5
v15.0.3-shopify-10	v	v16.0.7

The "previous" value for v15.0.3-shopify-10 looks suspicious but I suspect it's because we were not following the tag hygiene previous to v15. (I don't see any such tags for v14 for example). Seems fine? But would appreciate tallying with an expected table.

pawandubey · 2024-03-07T18:35:39Z

tools/get_next_release_shopify.sh

+
+target_release=""
+
+latest_major_release=$(git show-ref | grep -E 'refs/remotes/origin/v[0-9]*.[0-9]*.[0-9]*-shopify-[0-9]*$' | sed 's/[a-z0-9]* refs\/remotes\/origin\/v//' | sed 's/\.0//' | sort -nr | head -n1)


It's a bit unclear to me what major_release means here. Is it just the 16 part of 16.0.7-shopify-1 for example? Or is it 16.0.7-shopify-1 in its entirety?

Also should we do sort -Vr instead? Currently it sorts *-shopify-10 after *-shopify-9.

major_release is the 16 in 16.0.7-shopify-1. I think it makes more sense to look at latest major releases from upstream instead of shopify releases so I've updated that.

Essentially, it's used to compare with the major version that the PR is merging unto. If it's the same, then it uses the main branch to do the upgrade tests.

Having it only look at Shopify releases might lead to picking incorrect versions since if our latest release is, for example, v18 and we make a patch for v18, it would incorrectly pick main to compare with (instead of v19)

tools/get_next_release_shopify.sh

shanth96 · 2024-03-07T19:46:55Z

Would be great if there were some tophatting instructions, with e.g. what would the expected output of the two scripts be if we were checked out on a given version. For now I have:

That's a good call. I'll post some 🎩 instructions

shanth96 · 2024-03-11T15:10:54Z

🎩 instructions

Go to go/vt/servenv/version.go and update versionName to the value in version.go below.
Run ./tools/get_next_release_shopify.sh and ./tools/get_previous_release_shopify.sh
Compare the output with the respective columns below

Description	version.go	previous_release	next_release
Where previous release doesn’t exist	15.0.3		16.0.7-shopify-1
Where previous release exists but not a shopify next release	16.0.7	15.0.3-shopify-10	17.0.5
Where previous exist but on latest already for next	18.0.0		main

The empty string in previous releases is expected since we don't have any shopify releases that would match. This is okay because whenever we make a patch from now on, we will be guaranteed to have a previous shopify release.

brendar · 2024-03-13T17:58:27Z

tools/get_next_release_shopify.sh

+
+target_release=""
+
+latest_major_release=$(git show-ref | grep -E 'refs/remotes/origin/release-[0-9]*\.0$' | sed 's/[a-z0-9]* refs\/remotes\/origin\/release-//' | sed 's/\.0//' | sort -Vr | head -n1)


Was just testing this after pushing the latest v19.0.0 release tag (see https://github.com/Shopify/vitess-build/pull/58). This line is returning 18, not 19, because it's looking for release branches in our repo (which would need to be manually synced each time a new one is created upstream). Maybe this should look at tags instead?

I figured we sync major release branches into our repo since I saw the other releases but if that's not usually the case, makes sense to look at tags instead.

brendar

🎉

Run all relevant tests on all PRs (cherry picked from commit b55dd26)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14)

run make generate_ci_workflows similar to #148 remove custome machines for runs-on run make generate_ci_workflows use ubuntu

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08) (cherry picked from commit 33b5962)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08) (cherry picked from commit 33b5962) (cherry picked from commit a7ad1c7)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08) (cherry picked from commit 33b5962)

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08) (cherry picked from commit 33b5962) (cherry picked from commit a7ad1c7) (cherry picked from commit 1435f6f)

shanth96 requested review from pawandubey, brendar and shivnagarajan March 5, 2024 20:12

shanth96 force-pushed the fix-tests branch from 235b5c9 to 7ce4a66 Compare March 6, 2024 16:39

shanth96 force-pushed the fix-tests branch 4 times, most recently from 46643b7 to 008b78f Compare March 6, 2024 19:43

shanth96 force-pushed the fix-tests branch 2 times, most recently from 4a49f76 to 07b1add Compare March 6, 2024 20:13

pawandubey reviewed Mar 7, 2024

View reviewed changes

shanth96 force-pushed the fix-tests branch from 3a9c8f4 to fc795ee Compare March 7, 2024 19:34

shanth96 force-pushed the fix-tests branch from c13eb5d to ea216c3 Compare March 11, 2024 15:09

shanth96 requested a review from pawandubey March 11, 2024 16:55

shanth96 added 4 commits March 11, 2024 13:00

add release scripts

938a980

update skip workflow logic

0017d29

generate workflows

3beae36

manually update skip logic

ca026a1

shanth96 force-pushed the fix-tests branch from ea216c3 to ca026a1 Compare March 11, 2024 17:02

shanth96 changed the title ~~Run all relevant tests on release branches~~ Run all relevant tests on all branches Mar 11, 2024

shanth96 changed the title ~~Run all relevant tests on all branches~~ Run all relevant tests on all PRs Mar 11, 2024

brendar reviewed Mar 13, 2024

View reviewed changes

get latest release from tags

876fdf7

shanth96 requested a review from brendar March 13, 2024 19:23

brendar approved these changes Mar 13, 2024

View reviewed changes

shanth96 merged commit b55dd26 into v16.0.7-shopify-1-candidate Mar 14, 2024
221 of 223 checks passed

shanth96 added the Available: nil label Mar 14, 2024

shanth96 added a commit that referenced this pull request Mar 21, 2024

Merge pull request #148 from Shopify/fix-tests

8feee14

Run all relevant tests on all PRs (cherry picked from commit b55dd26)

shanth96 added the #gsd:38471 label Apr 1, 2024

shivnagarajan mentioned this pull request May 10, 2024

fix tests for CI #164

Merged

5 tasks

shanth96 added a commit that referenced this pull request May 10, 2024

Merge pull request #148 from Shopify/fix-tests

6bc3c08

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14)

shivnagarajan added a commit that referenced this pull request May 10, 2024

fix tests for CI

5cf9f24

run make generate_ci_workflows similar to #148 remove custome machines for runs-on run make generate_ci_workflows use ubuntu

shivnagarajan pushed a commit that referenced this pull request May 14, 2024

Merge pull request #148 from Shopify/fix-tests

33b5962

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08)

shivnagarajan pushed a commit that referenced this pull request May 19, 2024

Merge pull request #148 from Shopify/fix-tests

140a0c9

Run all relevant tests on all PRs (cherry picked from commit b55dd26) (cherry picked from commit 8feee14) (cherry picked from commit 6bc3c08)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run all relevant tests on all PRs #148

Run all relevant tests on all PRs #148

shanth96 commented Mar 5, 2024 •

edited

Loading

brendar commented Mar 5, 2024

brendar commented Mar 5, 2024

shanth96 commented Mar 6, 2024

shanth96 commented Mar 6, 2024

brendar commented Mar 6, 2024

shanth96 commented Mar 6, 2024

brendar commented Mar 6, 2024

shanth96 commented Mar 7, 2024

pawandubey left a comment

pawandubey Mar 7, 2024

shanth96 Mar 7, 2024

shanth96 commented Mar 7, 2024

shanth96 commented Mar 11, 2024

brendar Mar 13, 2024

shanth96 Mar 13, 2024

brendar left a comment


		target_release=""

		latest_major_release=$(git show-ref \| grep -E 'refs/remotes/origin/v[0-9].[0-9].[0-9]-shopify-[0-9]$' \| sed 's/[a-z0-9]* refs\/remotes\/origin\/v//' \| sed 's/\.0//' \| sort -nr \| head -n1)

Run all relevant tests on all PRs #148

Run all relevant tests on all PRs #148

Conversation

shanth96 commented Mar 5, 2024 • edited Loading

🎩 instructions

brendar commented Mar 5, 2024

brendar commented Mar 5, 2024

shanth96 commented Mar 6, 2024

shanth96 commented Mar 6, 2024

brendar commented Mar 6, 2024

shanth96 commented Mar 6, 2024

brendar commented Mar 6, 2024

shanth96 commented Mar 7, 2024

pawandubey left a comment

Choose a reason for hiding this comment

pawandubey Mar 7, 2024

Choose a reason for hiding this comment

shanth96 Mar 7, 2024

Choose a reason for hiding this comment

shanth96 commented Mar 7, 2024

shanth96 commented Mar 11, 2024

🎩 instructions

brendar Mar 13, 2024

Choose a reason for hiding this comment

shanth96 Mar 13, 2024

Choose a reason for hiding this comment

brendar left a comment

Choose a reason for hiding this comment

shanth96 commented Mar 5, 2024 •

edited

Loading