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

compose: Fix handling of base rev #3562

Merged
merged 1 commit into from
Apr 1, 2022
Merged

Conversation

cgwalters
Copy link
Member

I definitely tested the previous patch (and so did Joseph)
but I am fearing that I did further cleanup before pushing
that broke it.

What we're seeing right now is a segfault trying to use this with
RHCOS (which notably doesn't have a ref in the treefile).

Anyways, we should only be trying to resolve the base rev in
the "outside" path, not the in-container path. (Right now, but
once we can depend on a newer ostree inside the container we can
actually make this all work symmetrically)

I definitely tested the previous patch (and so did Joseph)
but I am fearing that I did further cleanup before pushing
that broke it.

What we're seeing right now is a segfault trying to use this with
RHCOS (which notably doesn't have a `ref` in the treefile).

Anyways, we should only be trying to resolve the base rev in
the "outside" path, not the in-container path.  (Right now, but
once we can depend on a newer ostree inside the container we can
actually make this all work symmetrically)
@cgwalters
Copy link
Member Author

Looking at the PR history, I think what happened is:

  • I got the code to work in the container path
  • I went back and fixed all the things I broke in the "outside i.e. coreos-assembler" path
  • I apparently didn't re-test the container path correctly

https://github.com/coreos/rpm-ostree/compare/cec7714624b2764b4fc68cae8b6123e29835485e..526140928a7e595430cfebe3b374e9c577930d54#
specifically moved the call to ostree_repo_resolve_rev() outside the conditional incorrectly.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Adding a test for the container path here would be good.

@jmarrero jmarrero self-requested a review March 31, 2022 20:08
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

tested and lgtm

@cgwalters
Copy link
Member Author

Adding a test for the container path here would be good.

Yeah definitely...I was hoping to wire things up more in a real e2e fashion alongside openshift/os#763

(Basically we start testing rhcos builds here too, including this container extensions flow)

@lucab lucab merged commit 1869937 into coreos:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants