-
Notifications
You must be signed in to change notification settings - Fork 239
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
fix(chart/data-prepper) Remove demo pipeline from values, add explicit demo value #634
base: main
Are you sure you want to change the base?
fix(chart/data-prepper) Remove demo pipeline from values, add explicit demo value #634
Conversation
Signed-off-by: Jan Høydahl <[email protected]>
Adding @SergK to please take a look as well. |
Thanks @janhoy I like the approach to use |
Signed-off-by: Aaron Layfield <[email protected]>
… remove-demo-pipeline-from-values
Signed-off-by: Aaron Layfield <[email protected]>
@janhoy I've pushed out the updates to hopefully get the linter passing. FYI, by default this would be a breaking change for anybody upgrading. Specifically because the default stance of the My recommendation is we keep existing behaviours and have the This is why the CI Pipelines are failing as well:
|
… configured. Signed-off-by: Jan Høydahl <[email protected]>
Yes, it was breaking change. I modified the behavior so that the demo pipeline is installed automatically only when no pipeline-secret or inline pipeline is enabled. The I do not see a benefit in preserving the possibility of ADDING a demo pipeline to an existing one, it would also be quite complicated. The "simple-sample-pipeline" is commented in values.yaml, so users can achieve this by modifying their own pipeline. |
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.
LGTM. Agree with the above.
Description
This PR comments demo pipline from values, and also removes the alread-commented complex pipeline example, refering to documentation instead.
Now, when installing the chart with defaults only, the install will fail with this message:
If you instead install the chart with
--set pipelineConfig.demoPipeline=true
, the behavior will be identical to current chart behavior, and we also print an informative message in stdout about what pipeline configuration is in use:Issues Resolved
Fixes #624
Check List
For any changes to files within Helm chart directories:
CHANGELOG.md
updated to reflect changeBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.