-
Notifications
You must be signed in to change notification settings - Fork 16
ACM-24258: handle K8s charts #356
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
ACM-24258: handle K8s charts #356
Conversation
scripts/build.sh
Outdated
| @@ -1,6 +1,7 @@ | |||
| #!/bin/bash | |||
| set -e | |||
| set -ex | |||
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.
let's remove x for the final version
| kustomize_build "$PROJECT" "${output_suffix}" | ||
| } | ||
|
|
||
| tmp_output_dir=$(mktemp -d -p ${WKDIR}) |
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 is no out dir in clean checkout :(
| tmp_output_dir=$(mktemp -d -p ${WKDIR}) | |
| mkdir -p ${WKDIR} | |
| tmp_output_dir=$(mktemp -d -p ${WKDIR}) |
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.
mkdir -p will create all necessary folder in the tree
edit: you're right, mktemp doesn't have the same behaviour
|
The cluster-api-provider-aws was updated by @mzazrivec & @serngawy ;) We can refresh |
cc38a21 to
c6814f6
Compare
| - name: NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace |
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.
Need to include the ASSISTED_CA_BUNDLE_* env vars to point to the secret inside the MCE target namespace...
- name: ASSISTED_CA_BUNDLE_NAME
value: assisted-installer-ca
- name: ASSISTED_CA_BUNDLE_NAMESPACE
value: {{ .Release.Namespace }}
- name: ASSISTED_CA_BUNDLE_RESOURCE
value: secret
- name: ASSISTED_CA_BUNDLE_KEY
value: ca.crt
| path: /spec/template/spec/containers/0/env/- | ||
| value: | ||
| name: ASSISTED_CA_BUNDLE_KEY | ||
| value: "ca.crt" |
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.
Patches are needed to ensure that capoa-bootstrap-validating-webhook-configuration reflects the target namespace...
- target:
version: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
name: capoa-bootstrap-validating-webhook-configuration
patch: |-
- op: replace
path: /metadata/annotations/cert-manager.io~1inject-ca-from
value: '{{ .Release.Namespace }}/capoa-bootstrap-cert'
- op: replace
path: /webhooks/0/clientConfig/service/namespace
value: '{{ .Release.Namespace }}'
| patch: |- | ||
| - op: replace | ||
| path: /metadata/annotations/cert-manager.io~1inject-ca-from | ||
| value: "{{ .Release.Namespace }}/capi-serving-cert" |
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.
Where is this value {{ .Release.Namespace }} will come from ?
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.
This will be rendered by the chart import process, and be swapped for NAMESPACE_PLACEHOLDER. It will then swapped back to the "release namespace" (i.e. multicluster-engine) when backplane-operator is going to render it. We discussed this choice with @cameronmwall , this is the recommended way to express "release namespace name"
| @@ -0,0 +1,6 @@ | |||
| $patch: delete | |||
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.
are we adding the Cert CR then deleting it ? why not keeping it as before not included and include it only with k8s chart.
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 thought having every change centralized in kustomize would be easier to maintain/follow compared to split logic throughout build.sh/sync2chart.sh and charts.
This is why we've added the hack script: so we can customize stuff within the config folder instead of some "complicated" generic logic.
If you do not agree we can find a better solution.
scripts/build.sh
Outdated
| if [ "$PROJECT" == "cluster-api" ] ; then | ||
| rm -rf $CONFIGDIR/$TMPDIR/apiextensions.k8s.io_v1_customresourcedefinition_ip*.yaml | ||
| # Helper function to build kustomize resources from a specific config directory | ||
| _kustomize_build() { |
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.
As there is kustomize_build func better name could _kustomize_config_build
scripts/sync2chart.sh
Outdated
| rm -rf "$chartdir/crds/*.yaml" | ||
| } | ||
|
|
||
| _sync_chart_files() { |
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.
| _sync_chart_files() { | |
| _move_chart_files() { |
scripts/sync2chart.sh
Outdated
| echo "generated helm template output to $k8s_output_file" | ||
| } | ||
|
|
||
| _generate_helm_template() { |
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.
| _generate_helm_template() { | |
| _save_helm_template() { |
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.
Is there a reason you repeating the functions names ?
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.
Not really, the logic was naming the "public" function, then abstracting within for handling the two types of config (OCP and k8s). As the semantic was just the same, but by config, I've just named the same as the parent function but with prepending _ (meaning private method, although I know it's not really private)
57f200c to
5800c26
Compare
Co-authored-by: Chris Wheeler <[email protected]> Signed-off-by: Riccardo Piccoli <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rccrdpccl, serngawy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
31ca846
into
stolostron:backplane-2.10
adding the possibility to publish k8s charts.
if the config folder
config-k8s/<PROJECT>is found, a new chart PROJECT-k8s will be generated incharts/