Skip to content
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

RBAC Migration Followup #10: Fix router ignore error #15822

Closed
enj opened this issue Aug 17, 2017 · 4 comments · Fixed by #15907
Closed

RBAC Migration Followup #10: Fix router ignore error #15822

enj opened this issue Aug 17, 2017 · 4 comments · Fixed by #15907

Comments

@enj
Copy link
Contributor

enj commented Aug 17, 2017

Fix ignoreError in pkg/oc/admin/router/router.go

@mrogers950
Copy link
Contributor

@enj it looks like you're referring to correcting the 'Temp fix for test/cmd/router.sh' part of 806d7be. What are we looking for in a permanent fix here?

@mrogers950
Copy link
Contributor

Removing the added check for details.Kind == "clusterrolebindings" does not cause test/cmd/router.sh to fail in any obvious way, so perhaps this fix is not needed anymore.

@mrogers950 mrogers950 self-assigned this Aug 22, 2017
@enj
Copy link
Contributor Author

enj commented Aug 22, 2017

@mrogers950 yeah I was hoping that reverting back the original code

return (details.Kind == "serviceaccounts" && details.Name == saName) ||
(details.Kind == "clusterrolebinding" && details.Name == roleBindingName)

would work since I had done a lot of work to make the proxy give the "correct" error.

I am fine with that as a fix, but see if you can figure out what ignoring that error actually means and add some docs around it in the code.

@mrogers950
Copy link
Contributor

The ignoreError functionality was added for https://bugzilla.redhat.com/show_bug.cgi?id=1332432 via #11639. I'll clarify the comments and also revert the original code.

openshift-merge-robot added a commit that referenced this issue Aug 24, 2017
Automatic merge from submit-queue (batch tested with PRs 15870, 15888, 15788, 15907, 15936)

Clarify router cmd ignoreError comments

fixes #15822
@enj @pweil-
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants