-
Notifications
You must be signed in to change notification settings - Fork 413
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
Add osimageurl to release payload and controllerconfig #305
Add osimageurl to release payload and controllerconfig #305
Conversation
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.
Looks good! Will defer to a second review for merging.
Mounting the configmap vs syncing? |
I did that originally, but how do you suggest monitoring for changes?
Can you flesh this out a bit? EDIT: I guess you're saying to pass the name of the CM and have the controller reread it? I don't quite understand the advantage; it would seem to require rewriting the renderer's queuing to know how to monitor the CM too, and that looked invasive (but I could be wrong). |
As a suggestion I think that we should refrain from making unrelated commits (like changing the SSH tests in osimageurl work) in PRs like this going forward bc they are hard to see/review and get lost in the shuffle. |
I generally agree, however I'm trying to batch at least some PRs to increase the chances they make it through the e2e-aws roulette. It is a clearly separate commit, and this is already a prep PR split from the bigger one. But if you still prefer we can separate it out. |
74ac9e5
to
9ce87a9
Compare
I agree with @kikisdeliveryservice's sentiment. For the time being to help move through CI I think it's OK to have PRs with unrelated commits but, when we do that, let's have the PR name reference all the changes so it's searchable. |
/retest |
OK so we don't have a whole lot of time left to implement OS updates. We can redesign it...review is good, but at the moment it's just me actively hacking on it, and right now to work on it one needs to redeploy the whole stack (operator, controller, server, daemon). It'd make it significantly more convenient for others to hack on if we landed these operator pieces now. |
These changes LGTM in light of the work I had to do on SSH. I'll let someone more familiar with the OS updates @jlebon @abhinavdahiya do the final approval. |
Split out of openshift#273 This is the first part of closing the gap in getting the RHCOS oscontainer into the update payload. Introduce a new ConfigMap machine-config-osimageurl that points to the oscontainer and is owned by the CVO. Then, the operator propagates that into the `controllerconfig` CRD. Further patches will have the controller react to the update, but having this first step in will help us validate the model without actually having clusters update.
9ce87a9
to
b5ec446
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon 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 |
simplify imagestream/template directory structure; more sensible dir name
[ And also the SSH test patch ]
Split out of #273
This is the first part of closing the gap in getting the RHCOS oscontainer
into the update payload.
Introduce a new ConfigMap machine-config-osimageurl that points to
the oscontainer and is owned by the CVO. Then, the operator propagates
that into the
controllerconfig
CRD.Further patches will have the controller react to the update, but having
this first step in will help us validate the model without actually
having clusters update.