Skip to content

fix: remove deprecated GetValueFromIntOrPercent method#2586

Open
fengshunli wants to merge 1 commit into
argoproj:masterfrom
fengshunli:vfp
Open

fix: remove deprecated GetValueFromIntOrPercent method#2586
fengshunli wants to merge 1 commit into
argoproj:masterfrom
fengshunli:vfp

Conversation

@fengshunli

Copy link
Copy Markdown
Member

Signed-off-by: fengshunli 1171313930@qq.com

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@github-actions

github-actions Bot commented Feb 11, 2023

Copy link
Copy Markdown
Contributor

Go Published Test Results

1 952 tests   1 952 ✔️  2m 35s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 8a72f89.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Feb 11, 2023

Copy link
Copy Markdown
Contributor

E2E Tests Published Test Results

    2 files      2 suites   1h 43m 50s ⏱️
  96 tests   89 ✔️ 3 💤 4
196 runs  186 ✔️ 6 💤 4

For more details on these failures, see this check.

Results for commit f6e6afc.

♻️ This comment has been updated with latest results.

@fengshunli fengshunli force-pushed the vfp branch 4 times, most recently from 14e0ca2 to f6e6afc Compare February 11, 2023 20:30
@fengshunli fengshunli changed the title fix: remove deprecated GetValueFromIntOrPercent method call fix: remove deprecated GetValueFromIntOrPercent method Feb 11, 2023
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov

codecov Bot commented Feb 11, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.98%. Comparing base (180300b) to head (395d422).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2586      +/-   ##
==========================================
+ Coverage   84.92%   84.98%   +0.06%     
==========================================
  Files         166      166              
  Lines       19047    19047              
==========================================
+ Hits        16176    16188      +12     
+ Misses       2008     2002       -6     
+ Partials      863      857       -6     
Flag Coverage Δ
e2e 52.83% <66.66%> (+0.10%) ⬆️
unit-tests 81.36% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zachaller

Copy link
Copy Markdown
Collaborator

Docs from deprecated function:

GetValueFromIntOrPercent was deprecated in favor of GetScaledValueFromIntOrPercent. This method 
was treating all int as a numeric value and all strings with or without a percent symbol as a
percentage value. Deprecate

This is a change in behavior that needs to be noted, it is still the correct thing to do because the old behavior is confusing but going to make a BREAKING CHANGE note in the sqash commit to note the behavior change.

@zachaller zachaller added this to the v1.5 milestone Feb 28, 2023
@zachaller zachaller modified the milestones: v1.5, v1.6 Mar 28, 2023
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
11.2% 11.2% Duplication

Signed-off-by: fengshunli <1171313930@qq.com>
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

Published E2E Test Results

  4 files    4 suites   3h 30m 28s ⏱️
120 tests 111 ✅  7 💤 2 ❌
482 runs  452 ✅ 28 💤 2 ❌

For more details on these failures, see this check.

Results for commit 395d422.

@github-actions

Copy link
Copy Markdown
Contributor

Published Unit Test Results

2 495 tests   2 495 ✅  3m 20s ⏱️
  130 suites      0 💤
    1 files        0 ❌

Results for commit 395d422.

@kostis-codefresh

kostis-codefresh commented Jun 22, 2026

Copy link
Copy Markdown
Member

@kostis-codefresh kostis-codefresh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fact that this is a breaking change was already mentioned by Zach and this is obviously very important.

However, I have an additional comment:

While this PR solves the problem itself, it silently consumes the error and never reports back if the user has done a mistake and entered "1" (i.e. an actual string). Or am I missing something here?

I think we should at least follow Argo CD and expose the error to the user

Ideally we should modify ValidateRollout and show a validation error (which is similar to what the standard K8s deployment controller will do)

Otherwise not only we break backwards compability, but we also don't notify existing users that something wrong is happening with their rollout spec.

@kostis-codefresh kostis-codefresh self-assigned this Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants