8000 fix(snyk-report): Improve calculation of latest patch release by olivergondza · Pull Request #23223 · argoproj/argo-cd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(snyk-report): Improve calculation of latest patch release #23223

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
Merged
merged 1 commit into from
Jun 3, 2025
Merged

fix(snyk-report): Improve calculation of latest patch release #23223

merged 1 commit into from
Jun 3, 2025

Conversation

olivergondza
Copy link
Contributor

The script is used by https://github.com/argoproj/argo-cd/actions/workflows/update-snyk.yaml

Last successful run is from March 16, on March 17 there was commit tagged with v3.0.0-rc1 breaking the calculation. The calculation used to rely on the fact that the minor version can be correctly decremented to get to the previous version.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • [n/a] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • [] Does this PR require documentation updates?
  • [n/a] I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • [n/a] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • [n/a] My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@olivergondza
fix(snyk-report): Improve calculation of latest patch release
ada3aa3 
This one does not choke on 3.0

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@olivergondza olivergondza requested a review from a team as a code owner June 2, 2025 09:22
@bunnyshell Bunnyshell
Copy link
bunnyshell bot commented Jun 2, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@olivergondza
Copy link
Contributor Author

This was verified manually - up to a point of actual snyk invocation. To be "fully" verified by the GH Actions invocation after merge.

nitishfy
nitishfy reviewed Jun 2, 2025
View reviewed changes
8000 Copy link
Member
@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

Few questions here:

  1. Did you try testing this on your fork?
  2. I'm a little confused about why the reports were not available although you mentioned the reason. Would you mind providing a better explanation for why the report was failing?

@olivergondza
Copy link
Contributor Author
olivergondza commented Jun 2, 2025 •
edited
Loading

I'm a little confused about why the reports were not available although you mentioned the reason. Would you mind providing a better explanation for why the report was failing?

Sure, before any report generation, it calculates recent 3-4 argo versions - latest patch of every minor "stream". To do so, it used to separate the 2 from, say, 14 and find previous stream by decrementing 14 and so on. This works well iff minor version number is above 4, that it was for quite some time, but it is not any longer on 3.0. So it ended up chocking on 3.-1 stream...

New impl takes latest tags from sorted lists, thus being agnostic to problems like that.

@olivergondza
Copy link
Contributor Author

Did you try testing this on your fork?

I did test that it produces the desirable version numbers to run the report against. What I did not test is the snyk service integration (code i did not touch) as I do not have the credentials / snyk account.

@nitishfy
Copy link
Member
nitishfy commented Jun 2, 2025

Did you try testing this on your fork?

I did test that it produces the desirable version numbers to run the report against. What I did not test is the snyk service integration (code i did not touch) as I do not have the credentials / snyk account.

Yep! That's the problem. Testing that thing on the fork is difficult due to lack of credentials. I'll let some other maintainer touch this PR. cc @rumstead

crenshaw-dev
crenshaw-dev approved these changes Jun 3, 2025
View reviewed changes
Copy link
Member
@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Manual tinkering looks okay.... gonna YOLO this, the report can't get any more broken. :-) Thanks for the PR!

@crenshaw-dev crenshaw-dev enabled auto-merge (squash) June 3, 2025 17:05
@crenshaw-dev crenshaw-dev merged commit 4b1bd18 into argoproj:master Jun 3, 2025
40 of 42 checks passed
@codecov Codecov
Copy link
codecov bot commented Jun 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.11%. Comparing base (81ac621) to head (ada3aa3).
Report is 7 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #23223      +/-   ##
==========================================
+ Coverage   60.09%   60.11%   +0.02%     
==========================================
  Files         342      342              
  Lines       57845    57845              
==========================================
+ Hits        34760    34775      +15     
+ Misses      20314    20311       -3     
+ Partials     2771     2759      -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crenshaw-dev
Copy link
Member

Huge improvement! https://github.com/argoproj/argo-cd/pull/23245/files

@olivergondza do you have time for a follow-up PR? A couple things to fix would be:

  1. re-introduce "master" to the generated list
  2. return to reverse-chronological order in the generated doc (i.e. most recent minor version first)

@olivergondza olivergondza deleted the fix-snyk-report branch June 4, 2025 06:23
@olivergondza
Copy link
Contributor Author

@crenshaw-dev, yeah, let me take a look

@olivergondza olivergondza mentioned this pull request Jun 4, 2025
Merged
8 tasks
chansuke pushed a commit to chansuke/argo-cd that referenced this pull request Jun 4, 2025
@olivergondza @chansuke
fix(snyk-report): Improve calculation of latest patch release (argopr…
9def7fa 
…oj#23223)

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
@olivergondza @dsuhinin
fix(snyk-report): Improve calculation of latest patch release (argopr…
4f84f7a 
…oj#23223)

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
@olivergondza @dsuhinin
fix(snyk-report): Improve calculation of latest patch release (argopr…
f757be3 
…oj#23223)

Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nitishfy nitishfy nitishfy left review comments

@crenshaw-dev crenshaw-dev crenshaw-dev approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@olivergondza @nitishfy @crenshaw-dev
0