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

Switch to https://github.com/coreos/rpmostree-client-go #3302

Merged

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Aug 22, 2022

We have a lot of ad-hoc code in the MCO interacting with rpm-ostree.
There's about 50% of it that uses this "NodeUpdaterClient" abstraction
which I guess in theory could have tried to also support e.g.
traditional dnf/apt systems or whatever, but never did - and
its API has grown things that make no sense on those systems (e.g.
"staged deployment" etc.)

The other half of the code ignores that abstraction (sensibly) and
just runs rpm-ostree via the CLI directly, crafting command
line arguments.

We now have https://github.com/coreos/rpmostree-client-go
which is intending to be "official" Go bindings which for now
just wrap the CLI, but do so in a more consistent way and offering
a cleaner API.


@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022
@cgwalters
Copy link
Member Author

If we go this route (and I think we should) a next step would be to just remove the NodeUpdaterClient abstraction; the mock unit tests we have basically aren't useful.

cgwalters added a commit to cgwalters/machine-config-operator that referenced this pull request Aug 23, 2022
Prep for openshift#3302

We should either use dependabot for openshift/ or use another
similar tool.
@cgwalters cgwalters requested a review from jkyros August 23, 2022 18:54
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2022
@jkyros
Copy link
Contributor

jkyros commented Aug 25, 2022

  • The realist in me says leave it how it is since it's currently less overhead for the MCO, and we'd likely be dependent on you to merge changes to this wrapper (not that you aren't responsive, you are very responsive, but you know what I mean)
  • The purist in me says yes, we should do this, so that rpm-ostree can expose things to us in the way they're meant to be used, and potentially other parties could benefit from using this wrapper

If we go this route (and I think we should) a next step would be to just remove the NodeUpdaterClient abstraction; the mock unit tests we have basically aren't useful.

Yes, I think we can probably work through what to do with the "node updater client" as we work through layering?

@cgwalters
Copy link
Member Author

and we'd likely be dependent on you to merge changes to this wrapper

We'll definitely move this into github.com/coreos/rpmostree-client-go - and anyone on the coreos team hence could review and merge PRs. (Not to mention, happy to give review/commit access to you too 😄 )

@cgwalters
Copy link
Member Author

Depends on #3325

@cgwalters cgwalters force-pushed the use-rpmostree-client-go branch 2 times, most recently from ad107e9 to 6294094 Compare September 28, 2022 18:54
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022
@cgwalters cgwalters changed the title WIP/RFC: Switch to https://github.com/cgwalters/rpmostree-client-go Switch to https://github.com/cgwalters/rpmostree-client-go Sep 28, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2022
@cgwalters
Copy link
Member Author

OK now that #3317 merged, I've rebased 🏄 this

@cgwalters
Copy link
Member Author

I've invited @jkyros from this team as a collaborator on https://github.com/coreos/rpmostree-client-go (and am happy to add others) to ensure the MCO team can always get timely and quick fixes into that repo.

(But that said again, nothing at all stops the MCO or anyone else from continuing to just run /usr/bin/rpm-ostree too)

@cheesesashimi
Copy link
Member

Just wanted to add that I like this change and am happy to see rpm-ostree get its own proper client!

@vrutkovs
Copy link
Member

vrutkovs commented Oct 1, 2022

/test okd-e2e-aws

@cgwalters
Copy link
Member Author

Friendly ping, seems like everyone's in favor, can someone drop a lgtm?

@sinnykumari
Copy link
Contributor

Don't see any concern with this move. Also happy that rpm-ostree will be having its own client lib which multiple project can directly use (including MCO)

Minor update in commit message and PR header
should be github.com/coreos/rpmostree-client-go/ instead of https://github.com/cgwalters/rpmostree-client-go/

We have a lot of ad-hoc code in the MCO interacting with rpm-ostree.
There's about 50% of it that uses this "NodeUpdaterClient" abstraction
which I guess in theory could have tried to also support e.g.
traditional dnf/apt systems or whatever, but never did - and
its API has grown things that make no sense on those systems (e.g.
"staged deployment" etc.)

The other half of the code ignores that abstraction (sensibly) and
just runs `rpm-ostree` via the CLI directly, crafting command
line arguments.

We now have https://github.com/coreos/rpmostree-client-go
which is intending to be "official" Go bindings which for now
just wrap the CLI, but do so in a more consistent way and offering
a cleaner API.
@cgwalters cgwalters changed the title Switch to https://github.com/cgwalters/rpmostree-client-go Switch to https://github.com/coreos/rpmostree-client-go Oct 7, 2022
@cgwalters
Copy link
Member Author

Ah yep, updated the commit and PR title.

@sinnykumari
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, sinnykumari

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:
  • OWNERS [cgwalters,sinnykumari]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 7, 2022

@cgwalters: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws bf3196870e0bc2b697ecf0a037dc83b555cb8d79 link false /test okd-e2e-aws
ci/prow/okd-scos-e2e-aws 3e58742 link false /test okd-scos-e2e-aws
ci/prow/okd-scos-e2e-vsphere 3e58742 link false /test okd-scos-e2e-vsphere
ci/prow/okd-scos-e2e-gcp-op 3e58742 link false /test okd-scos-e2e-gcp-op
ci/prow/okd-scos-e2e-upgrade 3e58742 link false /test okd-scos-e2e-upgrade

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/test-infra repository. I understand the commands that are listed here.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants