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

Add analysis to RolloutExperimentStep #238

Merged
merged 2 commits into from
Oct 30, 2019
Merged

Conversation

dthomson25
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #238 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   85.26%   85.28%   +0.02%     
==========================================
  Files          49       49              
  Lines        4390     4398       +8     
==========================================
+ Hits         3743     3751       +8     
  Misses        460      460              
  Partials      187      187
Impacted Files Coverage Δ
rollout/analysis.go 85.9% <100%> (ø) ⬆️
rollout/experiment.go 84.16% <100%> (+1.13%) ⬆️

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 d6a9ffd...4327aa8. Read the comment docs.

@@ -152,7 +152,8 @@ type RolloutExperimentStep struct {
Duration int32 `json:"duration"`
// Templates what templates that should be added to the experiment. Should be non-nil
Templates []RolloutExperimentTemplate `json:"templates"`
//ProgressingDeadlineSeconds Is it ncessary?
//AnalysisTemplate what analysis to run with the experiment
Analysis *RolloutAnalysisStep `json:"analysis,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a list of []ExperimentAnalysisTemplateRef and be named analyses? This approach means we only support one analysis per experiment step.

Copy link
Member Author

@dthomson25 dthomson25 Oct 30, 2019

Choose a reason for hiding this comment

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

I agree with having multiple analyses, but the ExperimentAnalysisTemplateRef doesn't work because it doesn't allow us to inject dynamic values (like podHash) into the analysis.

@dthomson25 dthomson25 force-pushed the add-analysis-to-exp-step branch from 26f77e6 to 4327aa8 Compare October 30, 2019 18:20
@jessesuen jessesuen merged commit 3fb19c3 into master Oct 30, 2019
@dthomson25 dthomson25 deleted the add-analysis-to-exp-step branch November 17, 2019 21:04
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.

2 participants