Open zfrhv opened 2 months ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: zfrhv Once this PR has been reviewed and has the lgtm label, please assign adrianludwin for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Hi @zfrhv. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
@mochizuki875 can you please tell if you like it?
@zfrhv
Thank you for your suggestion and PR!
There is just one thing to note.
Now, to generate Helm Chart, we need to execute make charts
.
It is for keeping up-to-date with changes in HCN manifests(manifests/*.yaml
generated by make manifests
), which was barrier to create Helm chart previously.
Then, all files in charts/hnc/templates
are re-generated by make charts
like this, so you should not edit charts/hnc/templates/xxx.yaml
directly.
Sorry, I should left comment somewhere...
So in this case, it's good to update hack/helm_patches/update-helm.sh
to update template like that.
@mochizuki875
thank you for mentioning.
I updated the files accordingly, after running make charts
it did updated the templates as expected.
i didnt added the make charts
result to the PR because it also modified many other files
let me know if you want me to include the make charts
result in the PR
@zfrhv
IMO, I think it's better to add make charts
result if it works as expected, because I think latest charts are expected to be in charts
dir of project and some users may use the chart without make charts
.
Ideally, make charts
should be executed automatically and the helm chart should be updated to charts
dir and github pages as commented here, but I’ve not started working yet...
in values.yaml the image repository seems to be broken am I supposed to dig in and figure out why?
Have you set some environment values? (see here)
By setting these values as below,
$ export HNC_REGISTRY=gcr.io/k8s-staging-multitenancy
$ export HNC_IMG_NAME=hnc-manager
$ export HNC_IMG_TAG=v1.1.0
values.yaml should be generated like this as a result of make charts
.
image:
repository: gcr.io/k8s-staging-multitenancy/hnc-manager
tag: v1.1.0
imagePullPolicy: {}
...
sorry for making you feed me with a spoon
/ok-to-test
@zfrhv Thank you for your work! This PR seems to make sense. /lgtm /cc @rjbez17
Related this PR, I have two questions. If you have some information, please let me know.
Some ClusterRoles(admin, edit, view) templates are replaced from admin-role in this PR. However, there doesn’t seems to be Binding resource to bind them. Is this intentional? It comes from #318.
(NIT) At first, I thought the range operation should be used for --included-namespace-regex
like --excluded-namespace.
However, includedNamespacesRegex
which comes from --included-namespace-regex
is defined as string not arrayArg, so we can't.
How do you think?
@mochizuki875
rbac.authorization.k8s.io/aggregate-to-edit: "true"
which kubernetes uses to merge the cluster role into the default clusterRole edit
.
its just better permissions assignment instead of just giving admin to admin@zfrhv Thanks your information;)
Adding
--included-namespace-regex
option to the helm chart.as mentioned in the docs there is only
--included-namespace-regex
or--excluded-namespace
.so in order to exclude all
openshift.*
namespaces I used the--included-namespace-regex
as mentioned in this issue, this is added as an example invalues.yaml