-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 spark-rbac #1986
fix spark-rbac #1986
Conversation
+1, had the same issue this morning. |
@@ -49,7 +49,6 @@ subjects: | |||
roleRef: | |||
kind: Role | |||
name: spark-role | |||
namespace: {{ $jobNamespace }} |
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.
@AndrewChubatiuk , the support for multiple namespaces was introduced in this commit: GitHub commit link. Have you had a chance to verify if it's working as expected for you? You can also check out the PR raised by @Aransh for more details.
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.
it's not needed, thanks for removing it
@Aransh, could you resolve the conflicts so we can proceed with merging the PR? Thank you! |
Signed-off-by: Aran Shavit <[email protected]>
Signed-off-by: Aran Shavit <[email protected]>
@vara-bonthu bumped version for this and #1976 |
It does look like the checks are still failing. Could you please run the following in your local machine? https://github.com/kubeflow/spark-operator/blob/master/docs/developer-guide.md#run-helm-chart-lint and We may need to document this better in PR templates |
Signed-off-by: Aran Shavit <[email protected]>
@vara-bonthu yup seems I'm a bit lost with this 😵💫 |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Aransh, vara-bonthu 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 |
* remove non-existent field Signed-off-by: Aran Shavit <[email protected]> * bump version Signed-off-by: Aran Shavit <[email protected]> * README Signed-off-by: Aran Shavit <[email protected]> --------- Signed-off-by: Aran Shavit <[email protected]>
* remove non-existent field Signed-off-by: Aran Shavit <[email protected]> * bump version Signed-off-by: Aran Shavit <[email protected]> * README Signed-off-by: Aran Shavit <[email protected]> --------- Signed-off-by: Aran Shavit <[email protected]>
* remove non-existent field Signed-off-by: Aran Shavit <[email protected]> * bump version Signed-off-by: Aran Shavit <[email protected]> * README Signed-off-by: Aran Shavit <[email protected]> --------- Signed-off-by: Aran Shavit <[email protected]>
Purpose of this PR
A recent PR has added support for multiple namespace.
Among the changes it has added the field roleRef.namespace to a roleBinding, this field does not exist and causes issues with deployments. It is also not needed as the roleBinding resource is namespaced to begin with.
You can see a similar discussion here - https://stackoverflow.com/questions/51961432/cant-to-add-namespace-field-to-roleref-in-rolebinding
Proposed changes:
Change Category
Indicate the type of change by marking the applicable boxes:
Rationale
Checklist
Before submitting your PR, please review the following:
Additional Notes