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

Fixing BuildManager in order not to create a system:image-puller role for the authenticated users when the master and build namespaces are set to the same value #537

Merged

Conversation

fabiobrz
Copy link
Contributor

@fabiobrz fabiobrz commented Apr 11, 2023

Fix #536

Tests passed locally.


Please make sure your PR meets the following requirements:

  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • Code is self-descriptive and/or documented

… for the authenticated users when the master and build namespaces are set to the same value
@fabiobrz fabiobrz marked this pull request as ready for review April 11, 2023 10:24
@fabiobrz fabiobrz requested review from mnovak1 and simkam April 11, 2023 10:25
@fabiobrz fabiobrz marked this pull request as draft April 11, 2023 15:05
Copy link
Contributor

@mnovak1 mnovak1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however we'll need some comparing CI runs with same and different namespaces.

@fabiobrz
Copy link
Contributor Author

LGTM, however we'll need some comparing CI runs with same and different namespaces.

Thanks @mnovak1 - I'll try to collect and share some link to upstream runs in here.

@fabiobrz fabiobrz marked this pull request as ready for review April 12, 2023 10:13
@mnovak1
Copy link
Contributor

mnovak1 commented May 4, 2023

@fabiobrz do we have a passing ci run?

@fabiobrz
Copy link
Contributor Author

fabiobrz commented May 4, 2023

@fabiobrz do we have a passing ci run?

HI @mnovak1 - I released a XTF snapshot containing the fix, and submitted a couple of Intersmash PRs to check it, one uses the same name for both the namespaces [1], and one is changing the one for the build namespace [2].
Let me know whether anything else is needed.

[1]
Intersmash/intersmash#44

[2]
Intersmash/intersmash#43

@fabiobrz fabiobrz requested a review from mnovak1 May 4, 2023 13:54
Copy link
Contributor

@mnovak1 mnovak1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging

@mnovak1 mnovak1 merged commit c133ec7 into xtf-cz:master May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is BuildManager wrongly creating a system:image-puller RoleBinding?
2 participants