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

daemon: Adapt to OS being pre-pivoted #151

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Oct 31, 2018

Now that RHCOS is pre-pivoted (see
openshift/os#307), we need to tweak the logic
here on how we handle osImageURL. Notably, we allow ://dummy from
the installer to always match the current image, regardless of whether
we're pivoted or not. In fact, that should've been the logic from the
start.

Closes: #150

@jlebon
Copy link
Member Author

jlebon commented Oct 31, 2018

/hold

Not tested yet.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 31, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2018
@ashcrow
Copy link
Member

ashcrow commented Oct 31, 2018

# github.com/openshift/machine-config-operator/pkg/daemon
pkg/daemon/daemon.go:328:3: not enough arguments to return
	have (bool)
	want (bool, error)
pkg/daemon/rpm-ostree.go:8:2: imported and not used: "github.com/openshift/machine-config-operator/vendor/github.com/golang/glog"
# github.com/openshift/machine-config-operator/pkg/daemon
pkg/daemon/daemon.go:328:3: not enough arguments to return
	have (bool)
	want (bool, error)
pkg/daemon/rpm-ostree.go:8:2:
[...]

Now that RHCOS is pre-pivoted (see
openshift/os#307), we need to tweak the logic
here on how we handle `osImageURL`. Notably, we allow `://dummy` from
the installer to always match the current image, regardless of whether
we're pivoted or not. In fact, that should've been the logic from the
start.

Closes: openshift#150
@jlebon
Copy link
Member Author

jlebon commented Oct 31, 2018

This is blocked on libvirt installations working again so that I can test this change.

@ashcrow
Copy link
Member

ashcrow commented Oct 31, 2018

Will hand off review to @sdemos, but this makes sense to me.

@jlebon
Copy link
Member Author

jlebon commented Nov 1, 2018

OK, now tested! ✔️

@ashcrow
Copy link
Member

ashcrow commented Nov 1, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@ashcrow
Copy link
Member

ashcrow commented Nov 1, 2018

@jlebon remove the hold when ready

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jlebon
Copy link
Member Author

jlebon commented Nov 1, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2018
@openshift-merge-robot openshift-merge-robot merged commit 5e5c272 into openshift:master Nov 1, 2018
osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021
@jlebon jlebon deleted the pr/adapt-pivot branch May 1, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants