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.

Sign up for GitHub

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

Jump to bottom

tools/osbuild-mpp: add mpp-resolve-ostree-commits helper #1399

Merged
merged 5 commits into from
Oct 16, 2023
Merged

tools/osbuild-mpp: add mpp-resolve-ostree-commits helper #1399

merged 5 commits into from
Oct 16, 2023

Conversation

dustymabe
Copy link
Contributor

See individual commits for rationale.

@dustymabe
Copy link
Contributor Author

The last commit is an attempt to add a test for this. I'm not sure the best way to go about doing that, but maybe let's use that as a starting point for discussion on how?

alexlarsson
alexlarsson reviewed Oct 10, 2023
View reviewed changes
tools/osbuild-mpp Outdated Show resolved Hide resolved
@dustymabe
Copy link
Contributor Author

for the CI failure:

+++ b/test/data/manifests/fedora-coreos-ostree-image.json
@@ -504,7 +504,7 @@
               "type": "org.osbuild.ostree",
               "origin": "org.osbuild.source",
               "references": {
-                "bc49c681f6c1f931db8547f7d18ee3b9fef5dc044ff475b0076d58a655479f90": {
+                "f2824f5801f1fb85c8dc08dd0e7394031362b504ec48a2658e8ea459f52209a3": {
                   "ref": "fedora/x86_64/coreos/stable"
                 }
               }
@@ -1175,7 +1175,7 @@
     },
     "org.osbuild.ostree": {
       "items": {
-        "bc49c681f6c1f931db8547f7d18ee3b9fef5dc044ff475b0076d58a655479f90": {
+        "f2824f5801f1fb85c8dc08dd0e7394031362b504ec48a2658e8ea459f52209a3": {
           "remote": {
             "url": "https://kojipkgs.fedoraproject.org/ostree/repo/"
           }

The fedora/x86_64/coreos/stable is going to keep getting updated in our OSTree remote as we do new releases. Not sure how to fix that.

@achilleas-k
Copy link
Member

for the CI failure:

+++ b/test/data/manifests/fedora-coreos-ostree-image.json
@@ -504,7 +504,7 @@
               "type": "org.osbuild.ostree",
               "origin": "org.osbuild.source",
               "references": {
-                "bc49c681f6c1f931db8547f7d18ee3b9fef5dc044ff475b0076d58a655479f90": {
+                "f2824f5801f1fb85c8dc08dd0e7394031362b504ec48a2658e8ea459f52209a3": {
                   "ref": "fedora/x86_64/coreos/stable"
                 }
               }
@@ -1175,7 +1175,7 @@
     },
     "org.osbuild.ostree": {
       "items": {
-        "bc49c681f6c1f931db8547f7d18ee3b9fef5dc044ff475b0076d58a655479f90": {
+        "f2824f5801f1fb85c8dc08dd0e7394031362b504ec48a2658e8ea459f52209a3": {
           "remote": {
             "url": "https://kojipkgs.fedoraproject.org/ostree/repo/"
           }

The fedora/x86_64/coreos/stable is going to keep getting updated in our OSTree remote as we do new releases. Not sure how to fix that.

For testing we generally use resources and content that we control and can update when we need to. For RPMs we snapshot repositories. For containers we have one or two in the gitlab registry of our project. We never figured out the equivalent for ostree commits. We would need to host a commit in a repo somewhere where we know it wont change until we want to update the tests.

achilleas-k
achilleas-k reviewed Oct 12, 2023
View reviewed changes
tools/osbuild-mpp Outdated Show resolved Hide resolved
@cgwalters
Copy link
Contributor

We never figured out the equivalent for ostree commits. We would need to host a commit in a repo somewhere where we know it wont change until we want to update the tests.

The problem in a nutshell. ostree repositories are just files on a webserver to be clear though.

That said, if it helps it's really easy to "encapsulate" an ostree commit in a container image, and then unencapsulate it. So actually everywhere we accept a direct ostree commit reference as input, we could also accept an encapsulated container, and then un-encapsulate it early on in the build process.

@dustymabe
Copy link
Contributor Author

dustymabe commented Oct 12, 2023 •
edited
Loading

That said, if it helps it's really easy to "encapsulate" an ostree commit in a container image, and then unencapsulate it. So actually everywhere we accept a direct ostree commit reference as input, we could also accept an encapsulated container, and then un-encapsulate it early on in the build process.

don't worry 😄 there's a PR for that coming soon, but really we just ostree container image deploy it.

@achilleas-k
Copy link
Member

That said, if it helps it's really easy to "encapsulate" an ostree commit in a container image, and then unencapsulate it. So actually everywhere we accept a direct ostree commit reference as input, we could also accept an encapsulated container, and then un-encapsulate it early on in the build process.

don't worry 😄 there's a PR for that coming soon, but really we just ostree container image deploy it.

I think it's okay to skip automated testing until we get this feature. A manual test should suffice for now.

@dustymabe
Copy link
Contributor Author

I think it's okay to skip automated testing until we get this feature. A manual test should suffice for now.

I think you are saying we can just drop the manifests: add mpp-resolve-ostree-commits example commit from this PR?

@dustymabe
inputs/ostree: leverage util/ostree library code
6408d9f 
Similar to the cleanups in 4e99e80, let's start using the library
code for the calls to ostree here.
@dustymabe
sources/ostree: leverage util/ostree library code
5351ced 
Similar to the cleanups in 4e99e80, let's start using the library
code for the calls to ostree here.
@dustymabe
osbuild/util/ostree: create setup_remote function
9bc43b9 
This moves the setup_remote function from the ostree source into
util/ostree. This is prep for sharing this function with an mpp
helper in the future.
@dustymabe
tools/osbuild-mpp: add mpp-resolve-ostree-commits helper
aed1112 
This will make it easier to resolve OSTree refs into commits similar
to how mpp-resolve-images works for container image references to
SHA256 digests.
@dustymabe
Copy link
Contributor Author

I think it's okay to skip automated testing until we get this feature. A manual test should suffice for now.

I think you are saying we can just drop the manifests: add mpp-resolve-ostree-commits example commit from this PR?

dropped the fedora-coreos-ostree-image.mpp.yaml from the latest upload.

@lukewarmtemp @dustymabe
tools/osbuild-mpp: Fix empty ostree commit object in deploy stage
8b26633 
This adds an early return to the `_process_ostree_commits` function to
prevent an empty ostree commit object from being created in the deploy
stage which causes violations to the deploy stage input schema.
@dustymabe dustymabe mentioned this pull request Oct 12, 2023
Merged
@achilleas-k achilleas-k self-requested a review October 12, 2023 20:53
achilleas-k
achilleas-k approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM. I'm good to merge if @alexlarsson is happy with the outcome of the ref vs target thread.

supakeen
supakeen approved these changes Oct 16, 2023
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Looks OK to me code-wise.

alexlarsson
alexlarsson approved these changes Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@alexlarsson alexlarsson left a comment

Choose a reason for hiding this comment

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

lgtm

@achilleas-k achilleas-k merged commit 980ca03 into osbuild:main Oct 16, 2023
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexlarsson alexlarsson alexlarsson approved these changes

@achilleas-k achilleas-k achilleas-k approved these changes

@supakeen supakeen supakeen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dustymabe @achilleas-k @cgwalters @alexlarsson @supakeen @lukewarmtemp