Integrate Experiments with Analysis #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
jessesuen
commented
Oct 19, 2019
•
jessesuen
commented
- Refactors Experiment controller to allow degraded replicasets to fail the experiment
- Allows multiple analysis templates to be invoked as part of an Experiment
Codecov Report
@@ Coverage Diff @@
## master #210 +/- ##
==========================================
+ Coverage 84.52% 84.82% +0.29%
==========================================
Files 47 47
Lines 3981 4244 +263
==========================================
+ Hits 3365 3600 +235
- Misses 440 455 +15
- Partials 176 189 +13
Continue to review full report at Codecov.
|
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
fa1ce45 to
15ec304
Compare
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
15ec304 to
94a00f0
Compare
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
9628142 to
38a0b38
Compare
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
38a0b38 to
47e7340
Compare
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
4c8e487 to
7c85d2e
Compare
jessesuen
changed the title
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
53cf865 to
de2d492
Compare
jessesuen
commented
Oct 25, 2019
jessesuen
commented
Oct 25, 2019
| // we ignore this update. | ||
| msg := fmt.Sprintf(conditions.ExperimentTimeOutMessage, experiment.Name) | ||
| condition := conditions.NewExperimentConditions(v1alpha1.ExperimentProgressing, corev1.ConditionFalse, conditions.TimedOutReason, msg) | ||
| conditions.SetExperimentCondition(&newStatus, *condition) |
There was a problem hiding this comment.
I removed TimeOut condition from experiments because this now fails the Experiment, and will be implicit by the Failed status. One less state to manage.
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
4fa1339 to
29cb1f3
Compare
dthomson25
reviewed
Oct 25, 2019
| go test -failfast -covermode=count -coverprofile=coverage.out ${TEST_TARGET} | ||
|
|
||
| .PHONY: coverage | ||
| coverage: test |
There was a problem hiding this comment.
I'm excited to start using this!
| @@ -158,12 +164,31 @@ func NewExperimentController( | |||
| // Two different versions of the same Replica will always have different RVs. | |||
| return | |||
| } | |||
| if defaults.GetReplicasOrDefault(newRS.Spec.Replicas) == defaults.GetReplicasOrDefault(oldRS.Spec.Replicas) && | |||
There was a problem hiding this comment.
Would we want to use this logic with rollouts too? We don't have to do it now, but it's just a thought
There was a problem hiding this comment.
I think so, it's a riskier change though. Not sure if we consider other things about the replicaset, like conditions.
|
Minor: can you write tests to cover https://codecov.io/gh/argoproj/argo-rollouts/pull/210/diff#D1-74 and https://codecov.io/gh/argoproj/argo-rollouts/pull/210/diff#D1-194 |
jessesuen
force-pushed
the
experiment-analysis-integration
branch
from
74ee8bf to
874c3e4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.