-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
added namespace field in the namespace scoped resource templates of helm chart #7256
Conversation
Hi @longwuyuan. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@longwuyuan Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: ValidationError(RoleBinding.roleRef): unknown field "namespace" in io.k8s.api.rbac.v1.RoleRef I'll leave the approval here, and once this can be merged/tests passed someone (@tao12345666333 @cpanato or @strongjz ) can issue a lgtm here :) /approve |
/area helm |
I will check the failed e2e test and update.
Thanks,
; Long
…On Mon, 21 Jun, 2021, 5:51 AM Jintao Zhang, ***@***.***> wrote:
/area helm
/assign
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7256 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGZVWTS3IFGY3R5ACI6VRDTT2AXFANCNFSM47AHKCRQ>
.
|
I found this https://stackoverflow.com/questions/51961432/cant-to-add-namespace-field-to-roleref-in-rolebinding#51962012 @tao12345666333, Should I will try to remove the field from role and rolebindings files ? |
I am able to reproduce the CI test error on minikube so will try to see if removing fields from role & rolebindings resources;
|
@@ -10,6 +10,7 @@ roleRef: | |||
apiGroup: rbac.authorization.k8s.io | |||
kind: Role | |||
name: {{ include "ingress-nginx.fullname" . }} | |||
namespace: {{ .Release.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.
please remove this one.
namespace
should be under metadata
here:
metadata: | |
labels: | |
{{- include "ingress-nginx.labels" . | nindent 4 }} | |
app.kubernetes.io/component: controller | |
name: {{ include "ingress-nginx.fullname" . }} |
➜ ~ kubectl explain RoleBinding.roleRef
KIND: RoleBinding
VERSION: rbac.authorization.k8s.io/v1
RESOURCE: roleRef <Object>
DESCRIPTION:
RoleRef can reference a Role in the current namespace or a ClusterRole in
the global namespace. If the RoleRef cannot be resolved, the Authorizer
must return an error.
RoleRef contains information that points to the role being used
FIELDS:
apiGroup <string> -required-
APIGroup is the group for the resource being referenced
kind <string> -required-
Kind is the type of resource being referenced
name <string> -required-
Name is the name of resource being referenced
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.
ok. Proves how stupid I am. Apologies for the noise. Fixing it now
You just wrote it in the wrong place. |
@tao12345666333, I moved the namespace field from roleRef to metdata but I did the edit right here on the github gui, so did it result in my commits not being squashed ? |
Don't worry. /label tide/merge-method-squash |
We can squash then on the merge via the bot, or you can squash with:
|
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: longwuyuan, rikatz, tao12345666333 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 |
I squashed on my laptop. But push failed
And suggested I do a pull. Then like the idiot that I am instead of force
push, I did a pull, and landed back into the merge commit pothole. I barely
recalled that bot magic for squashing was introduced recently, so saved my
skin, this time. So now I have to learn more and avoid this in future.
Thanks tons and gratitude for so much kindness. No limit to joy that I made
my first PR , even though so unclean. Honor of a lifetime. Gratitude.
Thanks,
; Long
…On Mon, 21 Jun, 2021, 4:30 PM Ricardo Katz, ***@***.***> wrote:
We can squash then on the merge via the bot, or you can squash with:
- git log (and grab the HEAD commit id)
- git rebase -i IDFROMABOVE
In this step, change your last commit from "pick" to "fixup"
- git log again and check there's only one commit
- git push -f origin
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7256 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGZVWQCOFPDMVSINLCOSJTTT4LWLANCNFSM47AHKCRQ>
.
|
/hold cancel Thanks for your effort on this @longwuyuan :) |
@tao12345666333 maybe we should release a new helm chart version with the latest corrections. If so, I suggest you or @longwuyuan step 5 of https://github.com/kubernetes/ingress-nginx/blob/master/RELEASE.md (remember to also update the Changelog) and once this gets merged, a new helm chart is available ;) |
OK,let me make a new release. |
What this PR does / why we need it:
User reported that the command helm template does not render namespace field, for the namespace scoped resources, of this controller's helm chart. So this PR adds the namespace spec field oneliner, in the helm template yaml files, of namespaced resources, of the ingress-nginx controller helm chart.
#7226
Types of changes
Which issue/s this PR fixes
fixes # #7226
How Has This Been Tested?
__$ git remote -v
origin [email protected]:longwuyuan/ingress-nginx.git (fetch)
origin [email protected]:longwuyuan/ingress-nginx.git (push)
__$ pwd
/.../ingress-nginx/charts
__$ ls
ingress-nginx
__$ helm template -n testns0 ingcontroller0 ./ingress-nginx
Checklist: