-
Notifications
You must be signed in to change notification settings - Fork 880
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: Add Service field to Rollout Experiment to allow service creation #2633
feat: Add Service field to Rollout Experiment to allow service creation #2633
Conversation
Signed-off-by: Daniel Del Rio <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2633 +/- ##
=======================================
Coverage 81.51% 81.52%
=======================================
Files 133 133
Lines 19859 19866 +7
=======================================
+ Hits 16188 16195 +7
Misses 2836 2836
Partials 835 835
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, shouldn't you pass template.Service.Name in CreateService call? instead of rs.Name
Can we also add it to the spec here https://argoproj.github.io/argo-rollouts/features/specification/ with a comment about it like weight has #optional etc |
Can we also add an e2e test where we set the name, I just want to make sure things still get cleaned up etc and all the other functions work that look up service. I took a quick peek at it seems we mostly use an annotation on the service to find services but it would be nice to run an e2e with name set. We have this already: https://github.com/argoproj/argo-rollouts/blob/master/test/e2e/functional/experiment-with-service.yaml |
Signed-off-by: Daniel Del Rio <[email protected]>
@alexef That makes sense. That said, I can't find documentation on including the Service field in an Experiment. Doesn't seem to be on the Experiment page. Should we also add that? |
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
…riments Signed-off-by: Daniel Del Rio <[email protected]>
… Experiment Signed-off-by: Daniel Del Rio <[email protected]>
1ad0f8a
to
7dcafa1
Compare
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Follow-up to: #2624
#2624 rolled back the change to allow rollout experiments with no weights to still create services. However, we still want the allow the users to choose to create services for their experiments even with no weight/traffic routing enabled. This PR makes it so that users can set the Service attribute for their rollout experiments (with an optional
name
field), giving them the ability to still create services without settingweight
.Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.