-
Notifications
You must be signed in to change notification settings - Fork 98
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
change to generate kustomize base files from helm chart #1634
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1634 +/- ##
==========================================
+ Coverage 63.54% 66.21% +2.67%
==========================================
Files 53 53
Lines 2584 2081 -503
==========================================
- Hits 1642 1378 -264
+ Misses 827 588 -239
Partials 115 115 ☔ View full report in Codecov by Sentry. |
I think this is a great change. |
@hhk7734 Would you mind trying to fix the CI build issues for this PR? |
I just rebased to origin/main without changing any code. :) |
/test eventing-upgrade-tests |
@@ -1,6 +1,6 @@ | |||
apiVersion: v1 | |||
name: knative-operator | |||
version: {{ version }} | |||
version: 1.10.1 |
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.
Why did you pin a specific version? I think we would like to release the charts based on different versions.
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.
I was thinking of the following user scenario:
- git clone
- checkout to the user wanted version
- edit values.yaml
helm install knative-operator ./config/charts/knative-operator
In this scenario, I guess, users
- rarely look at Charts.yaml.
- rarely modify tags or versions.
So I wrote down the operator version at the time of work. Should I revert it?
knative_operator: | ||
knative_operator: | ||
image: gcr.io/knative-releases/knative.dev/operator/cmd/operator | ||
tag: {{ tag }} | ||
tag: v1.10.1 |
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.
Why do you pin a specific version?
operator_webhook: | ||
image: gcr.io/knative-releases/knative.dev/operator/cmd/webhook | ||
tag: {{ tag }} | ||
tag: v1.10.1 |
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.
Why do you pin a specific version?
Can you please include the ability to add custom annotations to all of the objects? Our use case would be to bundle the operator with default knativeserving/eventing objects along with the kafka eventing controller within an argocd application and we would need to control install order via argo's sync-wave annotations. The kafka eventing controller does not offer a helm chart so we only have a manifest file which would run at a default order of 0. So the ability to say the operator's sync-wave itself is <0 would be great. |
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.
There are still a few namespace: default
references. Shouldnt these all be namespace: {{ include "knativeOperator.namespace" .}}
Hi! @ryan-dyer-sp
I plan to add patterns that are frequently seen in helm values, as shown below. However, if there are too many changes in this PR, the review will be difficult, so I think it would be better to proceed with the follow-up work once this PR is merged. commonLabels: {}
operator:
labels: {}
annotations: {}
podLabels: {}
Files with |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hhk7734 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This Pull Request is stale because it has been open for 90 days with |
Fixes #1546
Hi!
Changed Helm templates for more flexiblity and added scripts to generate kustomize default files from charts.
When I compared the operator-hub bundles before and after changes, I got the below.