-
Notifications
You must be signed in to change notification settings - Fork 461
MCO-1961: Introduce the OSImageStream controller #5454
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
Conversation
|
@pablintino: This pull request references MCO-1961 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pablintino 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 |
2258299 to
5175173
Compare
| } | ||
|
|
||
| func TestGetOSImageStreamSetByName(t *testing.T) { | ||
| osImageStream := &v1alpha1.OSImageStream{ |
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.
thought (non-blocking): Although none of the test cases appear to mutate osImageStream, it is possible that they could do so and affect the outcome of the other test cases here. A potential idea could be to move this into the test loop which guarantees it would always be a fresh instance and modify the test case struct to set it to nil.
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.
Done in a follow-up.
| defer klog.Info("Shutting down MachineConfigController-OSImageStreamController") | ||
|
|
||
| go func() { | ||
| err := ctrl.boot() |
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.
question: Is the intent here that ctrl.boot() will be executed once and set the necessary objects? How does it get re-run if the OSImageStream changes? Or will that be coming later? 😄
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.
So, for now, OSImageStream cannot change as the content is built from the available streams that can only change if the content the release image ships changes. If so, the CVO will rollout an update and this boot logic will run once to update the resource.
5175173 to
cd9b593
Compare
cd9b593 to
237de1a
Compare
|
@cheesesashimi This is the follow up PR for the requested changes: #5458 |
936bc8d to
679ced5
Compare
679ced5 to
a5a49e0
Compare
|
@pablintino: 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-sigs/prow repository. I understand the commands that are listed here. |
|
Replaced by #5476 |
- What I did
- How to verify it
- Description for the changelog