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

Implementation of adoption #66

Merged
merged 1 commit into from Oct 13, 2020
Merged

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Oct 6, 2020

So far we've supported updating systems that we installed,
but we also need to handle updating at least older CoreOS
systems.

This shares a lot of similarity with update; the biggest
difference is that we aren't sure which files we should
be managing. So given a pending update, we only replace
files that exist in that update.

Closes: #38

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

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

@cgwalters
Copy link
Member Author

Working on splitting out some prep work from this PR.

@cgwalters
Copy link
Member Author

Aaand the first actually did eat the grub.cfg which is exactly the kind of thing we need to handle in adoption vs update. Fixed now, tested explicitly and now we also reboot into the updated system too.

@cgwalters cgwalters mentioned this pull request Oct 7, 2020
@cgwalters
Copy link
Member Author

OK now this one just depends on #72

@cgwalters
Copy link
Member Author

/retest

@cgwalters cgwalters changed the title WIP: Implementation of adoption Implementation of adoption Oct 12, 2020
@cgwalters
Copy link
Member Author

OK, lifting WIP on this!

@cgwalters cgwalters marked this pull request as ready for review October 12, 2020 15:42
src/bootupd.rs Outdated Show resolved Hide resolved
src/bootupd.rs Outdated Show resolved Hide resolved
src/bootupd.rs Outdated Show resolved Hide resolved
src/bootupd.rs Outdated Show resolved Hide resolved
@lucab
Copy link
Contributor

lucab commented Oct 13, 2020

It looks reasonable overall. I've left a few minor comments and a suggestion to maybe tweak the trait slightly. I think this may be missing a cargo fmt pass too.

So far we've supported updating systems that we installed,
but we also need to handle updating at least older CoreOS
systems.

This shares a lot of similarity with `update`; the biggest
difference is that we aren't sure which files we should
be managing.  So given a pending update, we only replace
files that exist in that update.

Closes: coreos#38
@cgwalters
Copy link
Member Author

Comments addressed!

@cgwalters
Copy link
Member Author

cgwalters commented Oct 13, 2020

I think this may be missing a cargo fmt pass too.

cargo fmt was clean here - which section of the code made you think it was missing that out of curiosity?

EDIT: also our CI runs a cargo fmt check.

@lucab
Copy link
Contributor

lucab commented Oct 13, 2020

@cgwalters some incosistent newlines in impl EFI, I thought rustfmt was the one taking care of uniforming that.

Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@cgwalters
Copy link
Member Author

/refresh

@cgwalters cgwalters added the lgtm label Oct 13, 2020
@cgwalters
Copy link
Member Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 6d9d2e3 into coreos:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement adoption
4 participants