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.

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

fix: rollout paused longer than progressDeadlineSeconds would briefly degrade #1268

Merged
merged 9 commits into from
Jun 12, 2021
Merged

fix: rollout paused longer than progressDeadlineSeconds would briefly degrade #1268

merged 9 commits into from
Jun 12, 2021

Conversation

khhirani
Copy link
Contributor

Closes #1225

Signed-off-by: khhirani [email protected]

@khhirani khhirani marked this pull request as draft June 10, 2021 04:01
@codecov
Copy link

codecov bot commented Jun 10, 2021 •
edited
Loading

Codecov Report

Merging #1268 (2490cf6) into master (69c52a7) will decrease coverage by 0.03%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1268      +/-   ##
==========================================
- Coverage   81.39%   81.35%   -0.04%     
==========================================
  Files         106      106              
  Lines        9531     9597      +66     
==========================================
+ Hits         7758     7808      +50     
- Misses       1252     1260       +8     
- Partials      521      529       +8
Impacted Files Coverage Δ
pkg/kubectl-argo-rollouts/cmd/status/status.go 93.22% <85.71%> (-0.23%) ⬇️
rollout/sync.go 76.09% <100.00%> (ø)
rollout/trafficrouting/istio/istio.go 78.03% <0.00%> (-0.45%) ⬇️

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 69c52a7...2490cf6. Read the comment docs.

jessesuen
jessesuen reviewed Jun 10, 2021
View reviewed changes
pkg/kubectl-argo-rollouts/cmd/status/status.go Outdated Show resolved Hide resolved
rollout/sync.go Outdated Show resolved Hide resolved
@khhirani khhirani marked this pull request as ready for review June 11, 2021 01:47
@khhirani
Create e2e test
008780b 
Signed-off-by: khhirani <[email protected]>
@jessesuen
Copy link
Member

jessesuen commented Jun 11, 2021 •
edited
Loading

The e2e tests are failing because the github environment doesn't have the kubectl argo rollouts plugin installed.

Error: unknown command "argo" for "kubectl"
Run 'kubectl --help' for usage.

So you'll need to invoke the status plugin logic another way. You can follow how I use the Promote() logic of the plugin from the e2e tests.

jessesuen
jessesuen reviewed Jun 11, 2021
View reviewed changes
rollout/controller_test.go Outdated Show resolved Hide resolved
test/e2e/functional_test.go Outdated Show resolved Hide resolved
@jessesuen jessesuen changed the title fix: Fix status command exists before timeout fix: rollout paused longer than progressDeadlineSeconds would briefly degrade Jun 11, 2021
khhirani added 2 commits June 11, 2021 02:09
@khhirani
Refactor tests
daf2709 
Signed-off-by: khhirani <[email protected]>
@khhirani
Fix e2e test
2490cf6 
Signed-off-by: khhirani <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud 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
12.5% 12.5% Duplication

jessesuen
jessesuen approved these changes Jun 12, 2021
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait for e2e / unit tests to pass before merging (resubmit e2e as necessary due to flakiness)

@khhirani khhirani merged commit fe4bb1b into argoproj:master Jun 12, 2021
@khhirani khhirani deleted the status-command-exits-bug branch June 12, 2021 01:13
perenesenko pushed a commit to perenesenko/argo-rollouts that referenced this pull request Jun 22, 2021
@khhirani @perenesenko
fix: rollout paused longer than progressDeadlineSeconds would briefly…
57f2b10 
… degrade (argoproj#1268)

* Do not exit status cmd for ProgressDeadlineExceeded error

Signed-off-by: khhirani <[email protected]>

* Do not return status if RO is paused in sync.go

Signed-off-by: khhirani <[email protected]>
Signed-off-by: Andrii Perenesenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessesuen jessesuen jessesuen 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.

status command exits before timout
2 participants
@khhirani @jessesuen