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: Uplevel AnalysisRun status to Rollout status #578

Merged
merged 4 commits into from
Jul 31, 2020

Conversation

dthomson25
Copy link
Member

Closes #559

Copy link
Contributor

@khhirani khhirani left a comment

Choose a reason for hiding this comment

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

LGTM

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.

I think approach is fine. Lets just use a Status.AbortedAt timestamp to help indicate we reconciled the abort, instead of another boolean.

// CurrentBackgroundAnalysisRun indicates the analysisRun for the Background step
// TODO(Depreciated): Remove in v0.10
Copy link
Member

Choose a reason for hiding this comment

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

s/Depreciated/Deprecated

@@ -550,13 +561,25 @@ type CanaryStatus struct {
// +optional
StableRS string `json:"stableRS,omitempty"`
// CurrentStepAnalysisRun indicates the analysisRun for the current step index
// TODO(Depreciated): Remove in v0.10
Copy link
Member

Choose a reason for hiding this comment

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

s/Depreciated/Deprecated/

CurrentBackgroundAnalysisRun string `json:"currentBackgroundAnalysisRun,omitempty"`
// CurrentStepAnalysisRunStatus indicates the status of the current background analysis run
Copy link
Member

Choose a reason for hiding this comment

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

Comment should be CurrentBackgroundAnalysisRunStatus

// the controller needs to do some specific work when a Rollout is aborted. For example, the reconcileAbort is used
// to indicate if the Rollout should enter an aborted state when the latest AnalysisRun is a failure, or the controller
// has already put the Rollout into an aborted and should create a new AnalysisRun.
ReconciledAbort bool `json:"reconciledAbort,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Instead of ReconciledAbort bool, can we have AbortedAt *metav1.Time which gets cleared out whenever Status.Abort = true

@codecov-commenter
Copy link

Codecov Report

Merging #578 into master will decrease coverage by 0.04%.
The diff coverage is 90.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
- Coverage   85.80%   85.75%   -0.05%     
==========================================
  Files          87       87              
  Lines        8297     8376      +79     
==========================================
+ Hits         7119     7183      +64     
- Misses        844      857      +13     
- Partials      334      336       +2     
Impacted Files Coverage Δ
utils/analysis/helpers.go 90.27% <0.00%> (-4.85%) ⬇️
rollout/analysis.go 88.39% <95.45%> (+1.19%) ⬆️
rollout/bluegreen.go 82.87% <100.00%> (+0.16%) ⬆️
rollout/canary.go 85.93% <100.00%> (+0.05%) ⬆️
rollout/context.go 98.56% <100.00%> (+0.09%) ⬆️
rollout/pause.go 99.14% <100.00%> (+0.03%) ⬆️
rollout/sync.go 74.75% <100.00%> (+0.06%) ⬆️
utils/analysis/filter.go 97.72% <100.00%> (-2.28%) ⬇️

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 46086a4...39b5d31. Read the comment docs.

@dthomson25 dthomson25 merged commit 7689b1f into argoproj:master Jul 31, 2020
@dthomson25 dthomson25 deleted the better-message branch July 31, 2020 15:48
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.

Uplevel Status Information about Analysis to Rollout
4 participants