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
remove MCO manifests #5383
remove MCO manifests #5383
Conversation
LGTM |
/assign @staebler |
@yuvalk Can you add some color for why this is no longer needed? |
@staebler see bz https://bugzilla.redhat.com/show_bug.cgi?id=1978581 tl;dr: let me know if you want me to include this info in the commit msg itself |
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
The code looks good, I'll leave it to someone else to decide if it's a good idea (approve).
/unassign |
/retest |
b2a9001
to
069fac7
Compare
1f7d9b1
to
b3d5497
Compare
I would like to take a step back from the changes made in this PR and evaluate whether the installer should even be creating these manifests. I feel like this is leftover from the tectonic days. The machine-config-operator should be rendering any manifests that it needs during bootstrapping or including them as part of the release payload. The machine-config-operator is already including the manifest for the openshift-machine-config-operator namespace as part of the release payload. Is there some reason why the namespace needs to exist earlier? Even so, rendering it during bootstrap would accomplish the same thing and push the ownership of the manifest to the MCO, where it belongs. As an aside, the MCO uses a runlevel of 1 for the openshift-machine-config-operator namespace manifest that is included in the release payload. |
We are addressing that in openshift/machine-config-operator#2655 Let's try and remove the file altogether (from installer) |
b3d4f4e
to
2e5af82
Compare
/test e2e-aws-upgrade |
Looks like removing the manifest entirely works just fine. The changes look good to me. Clean up the commit message and the PR title, and I'll give it a lgtm. |
These are leftovers from the tectonic era. With current implementation it should all be rendered by machine-config-operator itself
2e5af82
to
dd1e198
Compare
@yuvalk: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler 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 |
this is no longer needed