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.

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

ci/ipsec: Fix downgrade version retrieval #30742

Merged
merged 1 commit into from Feb 28, 2024

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Feb 13, 2024

Figuring out the right "previous patch release version number" to downgrade to in print-downgrade-version.sh turns out to be more complex than expected 0 1 2 3.

This commit is an attempt to 1) fix issues with the current script and 2) overall make the script clearer, so we can avoid repeating these mistakes.

As for the fixes, there are two things that are not correct with the current version. First, we're trying to validate the existence of the tag to downgrade to, in case the script runs on top of a release preparation commit for which file VERSION has been updated to a value that does not yet contains a corresponding tag. This part of the script is actually OK, but not the way we call it in the IPsec workflow: we use fetch-tags: true but fetch-depth: 0 (the default), and the two are not compatible, a shallow clone results in no tags being fetched.

To address this, we retrieve the tag differently: instead of relying on "fetch-tags" from the workflow, we call "git fetch" from the script itself, provided the preconditions are met (we only run it from a Git repository, if the "origin" remote is defined). If the tag exists, either locally or remotely, then we can use it. Otherwise, the script considers that it runs from a release preparation Pull Request, and decrements the patch release number.

The second issue is that we would return no value from the script if the patch release is zero. This is to avoid any attempt to find a previous patch release when working on a development branch. However, this logics is incorrect (it comes from a previous version of the script where we would always decrement the patch number). After the first release of a new minor version, it's fine to have a patch number at 0. What we should check instead is whether the version ends with -dev.

This commit brings additional changes for clarity: more comments, and a better separation between the "get latest patch release" and "get previous stable branch" cases, moving the relevant code to independent functions, plus better argument handling. We also edit the IPsec workflow to add some logs about the version retrieved. The logs should also display the script's error messages, if any, that are printed to stderr.

Sample output from the script:

VERSION     Tag exists  Prevous minor   Previous patch release

1.14.3      Y           v1.13           v1.14.3
1.14.1      Y           v1.13           v1.14.1
1.14.0      Y           v1.13           v1.14.0
1.14.1-dev  N           v1.13           <error>
1.15.0-dev  N           v1.14           <error>
1.13.90     N           v1.12           v1.13.89  <- decremented
2.0.0       N           <error>         <error>
2.0.1       N           <error>         v2.0.0    <- decremented
2.1.1       N           v2.0            v2.1.0    <- decremented

Split diff view recommended to review the new version of print-downgrade-version.sh.

@qmonnet qmonnet added kind/bug/CI This is a bug in the testing code. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/ci This PR makes changes to the CI. needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch feature/ipsec Relates to Cilium's IPsec feature needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Feb 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.19 Feb 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.1 Feb 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.7 Feb 13, 2024
@qmonnet
Copy link
Member Author

qmonnet commented Feb 13, 2024

/ci-ipsec-upgrade

@qmonnet qmonnet force-pushed the pr/qmonnet/ipsec/patch-release-downgrade-fix-more branch from 1d2f9be to a3a7607 Compare February 14, 2024 11:56
@qmonnet
Copy link
Member Author

qmonnet commented Feb 14, 2024

/ci-ipsec-upgrade

@qmonnet
Copy link
Member Author

qmonnet commented Feb 14, 2024

Tested in CI, in particular for the “downgrade to latest patch version” job:

  • On main, the steps are skipped (no latest patch version on branch, so this is expected)
  • On v1.15, current HEAD, we downgrade to v1.15.0 (expected)
  • On v1.15, with a test commit to change VERSION's content to v1.15.1, we decrement and downgrade to v1.15.0 (expected)
  • On v1.15, with a test commit to change VERSION's content to v1.16.0, we skip the remaining steps (no way to decrement from v1.16.0 - this is the expected behaviour).

@qmonnet qmonnet marked this pull request as ready for review February 14, 2024 12:40
@qmonnet qmonnet requested review from a team as code owners February 14, 2024 12:40
@qmonnet
Copy link
Member Author

qmonnet commented Feb 14, 2024

/test

@michi-covalent michi-covalent added this to Needs backport from main in 1.15.2 Feb 14, 2024
@michi-covalent michi-covalent removed this from Needs backport from main in 1.15.1 Feb 14, 2024
@pchaigno pchaigno requested review from gentoo-root and removed request for pchaigno February 15, 2024 10:08
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.15 in 1.15.2 Feb 29, 2024
@YutaroHayakawa YutaroHayakawa mentioned this pull request Feb 29, 2024
2 tasks
@YutaroHayakawa YutaroHayakawa added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Feb 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.8 Feb 29, 2024
@YutaroHayakawa YutaroHayakawa mentioned this pull request Feb 29, 2024
1 task
@YutaroHayakawa YutaroHayakawa added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Feb 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.13 Feb 29, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Mar 1, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.15 in 1.15.2 Mar 1, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Mar 1, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.13 Mar 1, 2024
@pippolo84 pippolo84 added the backport/author The backport will be carried out by the author of the PR. label Mar 5, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.15 in 1.15.2 Mar 5, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.8 Mar 5, 2024
@pippolo84
Copy link
Member

@qmonnet just a heads up that I was unable to backport this to v1.12 because of the conflicts, so I added the backport-author label.

@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.12.19 Mar 5, 2024
@qmonnet qmonnet removed the backport/author The backport will be carried out by the author of the PR. label Mar 5, 2024
@aanm aanm moved this from Needs backport from main to Backport done to v1.14 in 1.14.7 Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. feature/ipsec Relates to Cilium's IPsec feature kind/bug/CI This is a bug in the testing code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-blocker/1.15 This issue will prevent the release of the next version of Cilium. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.13.13
Backport done to v1.13
1.14.7
Backport done to v1.14
1.14.8
Backport done to v1.14
1.15.2
Backport done to v1.15
Development

Successfully merging this pull request may close these issues.

None yet

7 participants