-
Notifications
You must be signed in to change notification settings - Fork 401
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
WIP: Use resource{apply,merge,read} functions provided by library-go where possible #829
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LorbusChris 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 |
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.
I think the mcoresourceapply
name is super hard for my eyeballs to read. And also there would be less changes if you left that name as-is resourceapply
and instead named the resourceapply from libgo something like goresourceapply
. less overall changes and clearer imo.
The added override in Gopkg.toml brings in a huge amount of changes in the second commit, i.e. when running |
/retest |
1 similar comment
/retest |
/skip |
/retest |
fac4030
to
0c2217e
Compare
Verify going OOM may need us to request more memory in the pod in openshift/release? Probably worth trying to figure out how much memory it's using. |
@@ -68,6 +68,7 @@ func createDiscoveredControllerConfigSpec(infra *configv1.Infrastructure, networ | |||
} | |||
|
|||
platform := "none" | |||
//nolint:staticcheck | |||
switch infra.Status.Platform { | |||
case configv1.AWSPlatformType: |
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.
strangely, this errors out on this branch, but not on master.
where possible, replacing the forked ones in `./lib/`. Add events.Recorder as libgoRecorder to Operator struct. Removed now unused files in `./lib/resource{apply,merge,read}`.
…switch `make verify` errors out here otherwise, and we are not ready yet to switch from deprecated infra.Status.Platform for platformStatus.type, yet.
This is now fully passing - I'd go with this PR and tackle the other library-go replacements in a follow up. I want this to fully get rid of our, now, super old fork and then we can try to understand a move to sigs.k8s.io/controller-runtime as @ericavonb suggested on Slack. |
ack. will tidy the commit messages and bit before the merge. |
To be clear, I don't think we need controller-runtime or librarygo for what we're doing. Here's what I think we should do: |
isn't that going against #817 (comment)? (apart from moving to library-go or not). I think here in MCO isn't about consistency, but rather flexibility of managing our own resources (as Abhinav highlighted in his comment above) |
Per #817 (comment):
The operand for the MCO are machine configurations and the server/daemons to facilitate that. As the operator reconciles its CR types, those CRD definitions would be requirements (along with the RBAC required to handle them) for the operator.
CRDs in particular can't be handled only by the operator without consideration to the rest of the clsuter because they are global objects. If anything needed to interact with one of them before the MCO ran, it would need that definition to exist and having more than one place attempt to create the definitions would be a much more difficult migration situation. This is why the openshift/api config CRDs are created via the cluster-config-operator* before other operators run and other operators include their CRDs in their payloads at earlier runlevels. * created by the CVO but included in a release via the cluster-config-operator payload. with the "create-only" annotation, you can utilize the CVO to create resources that you may want to modify later. |
@LorbusChris: PR needs rebase. 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 think a plus one for library-go (at least today) is about not having those helpers everywhere. See openshift/library-go#472 also, we're currently "broken", and having a single place for the helpers is at least beneficial to catch and fix stuff like that (I just noticed about that) |
will resurrect this PR on Monday! |
@LorbusChris: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
In an effort to clean up the MCO repo, closing old open PRs with no recent activity. Feel free to reopen. |
This is a first PR to partly tackle #819
- What I did
Added changes to use resource{apply,merge,read} functions provided by library-go where possible instead of the forked ones from ./lib/resource{apply,merge,read}.
Removed now unused files in ./lib/resource{apply,merge,read}.
Regenerated
- How to verify it
CI
- Description for the changelog