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

feat: Added delay button in the scaled down revision (#1355) #1804

Merged
merged 27 commits into from
Feb 9, 2022

Conversation

schakrad
Copy link
Contributor

@schakrad schakrad commented Jan 25, 2022

fix for #1355

Before the addition of button.
Screen Shot 2022-02-07 at 11 52 30 PM

After the addition of button to show the scaleDownDelay in seconds
Screen Shot 2022-02-08 at 12 00 49 AM

Signed-off-by: “schakradari” <“[email protected]”>

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, (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.

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #1804 (9acb544) into master (56a94f4) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1804      +/-   ##
==========================================
+ Coverage   82.31%   82.36%   +0.05%     
==========================================
  Files         116      119       +3     
  Lines       16371    16872     +501     
==========================================
+ Hits        13475    13897     +422     
- Misses       2217     2284      +67     
- Partials      679      691      +12     
Impacted Files Coverage Δ
utils/replicaset/replicaset.go 88.27% <0.00%> (-1.78%) ⬇️
rollout/canary.go 76.32% <0.00%> (-0.58%) ⬇️
utils/replicaset/canary.go 89.26% <0.00%> (-0.23%) ⬇️
rollout/trafficrouting/alb/alb.go 81.95% <0.00%> (-0.18%) ⬇️
rollout/experiment.go 84.13% <0.00%> (ø)
utils/service/service.go 100.00% <0.00%> (ø)
metricproviders/graphite/graphite.go 100.00% <0.00%> (ø)
pkg/kubectl-argo-rollouts/info/info.go 100.00% <0.00%> (ø)
metricproviders/prometheus/prometheus.go 100.00% <0.00%> (ø)
pkg/kubectl-argo-rollouts/cmd/get/get.go 72.00% <0.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56a94f4...9acb544. Read the comment docs.

@harikrongali
Copy link
Contributor

@schakrad can you cleanup the PR and have diff show only required changes

@schakrad schakrad marked this pull request as ready for review January 26, 2022 20:15
@harikrongali harikrongali requested review from rbreeze and alexmt and removed request for rbreeze January 26, 2022 22:27
ui/package.json Outdated
@@ -60,7 +60,7 @@
"sass": "^1.32.8",
"ts-loader": "^8.0.17",
"webpack-bundle-analyzer": "^4.4.0",
"webpack-cli": "^4.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change required?

Copy link
Member

Choose a reason for hiding this comment

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

+1

ui/yarn.lock Outdated
@@ -2256,22 +2256,22 @@
"@webassemblyjs/wast-parser" "1.9.0"
"@xtuc/long" "4.2.2"

"@webpack-cli/configtest@^1.0.3":
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if lock file needs to be committed. @alexmt is it fine to commit this file?

Copy link
Member

Choose a reason for hiding this comment

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

@harikrongali Lock file does need to be committed, but we should avoid bumping versions in any of these packages if possible. @schakrad I don't see any code changes that should require updating package versions, can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @rbreeze for confirmation. @schakrad please revert the package.json update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure Hari, I will check and revert back.

@harikrongali
Copy link
Contributor

Please add before and after snapshots as a part of the PR description.

Please fix CI checks: DCO & build
For DCO, you can squash commits and sign off on the last one

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

Code changes look good, my only request is to avoid changing package versions

ui/package.json Outdated
@@ -60,7 +60,7 @@
"sass": "^1.32.8",
"ts-loader": "^8.0.17",
"webpack-bundle-analyzer": "^4.4.0",
"webpack-cli": "^4.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

+1

ui/yarn.lock Outdated
@@ -2256,22 +2256,22 @@
"@webassemblyjs/wast-parser" "1.9.0"
"@xtuc/long" "4.2.2"

"@webpack-cli/configtest@^1.0.3":
Copy link
Member

Choose a reason for hiding this comment

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

@harikrongali Lock file does need to be committed, but we should avoid bumping versions in any of these packages if possible. @schakrad I don't see any code changes that should require updating package versions, can you confirm?

@sonarqubecloud
Copy link

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

@schakrad schakrad force-pushed the fix-1355 branch 3 times, most recently from 6babcef to bd414b8 Compare January 27, 2022 18:23
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
ui/src/app/components/Timer/timer.tsx Outdated Show resolved Hide resolved
ui/src/app/components/Timer/timer.tsx Outdated Show resolved Hide resolved
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: “schakradari” <[email protected]>
ui/src/app/components/pods/pods.tsx Outdated Show resolved Hide resolved
schakrad and others added 6 commits February 3, 2022 13:50
fix: use consistent versions of typescript and classnames with argo-ui library
Signed-off-by: “schakradari” <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 8, 2022

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 2 Code Smells

No Coverage information No Coverage information
3.1% 3.1% Duplication

@alexmt
Copy link
Contributor

alexmt commented Feb 8, 2022

LGTM.

@rbreeze can you please take a look one more time?

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

Just one last comment, can we revert package.json and yarn.lock changes?

@alexmt
Copy link
Contributor

alexmt commented Feb 8, 2022

@rbreeze changes in package.json are actually required. Apparently argo-ui/argo-cd/ui and argo-rollouts/ui were using different classnames versions. I temporary downgraded classnames to unblock this PR.

@rbreeze
Copy link
Member

rbreeze commented Feb 8, 2022

@alexmt Oh I see, thank you!

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexmt alexmt merged commit 459112b into argoproj:master Feb 9, 2022
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.

4 participants