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

[RFC] cmd: remove gen-manifests in favor of the images version #4152

Closed
wants to merge 1 commit into from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented May 16, 2024

This is a result of the outcome of
#4124

Achilleas suggested that instead of having a copy of gen-manifests
in osbuild-composer we could use the one from images. AFAICT nothing
in the code references gen-manifests except the test/README so
it seems safe to remove and just use images.

The README got updated to point to images and we will also
need to remove test/data/manifest at a later point (after
checking that nothing else depends on this anymore).

@mvo5
Copy link
Contributor Author

mvo5 commented May 17, 2024

Ok, this idea did not pan out - the format between osbuild-composer and images has changed for the gen-manifests command and they are no longer compatible:

$ go run github.com/osbuild/images/cmd/gen-manifests@b46ff4b53399523a3724a981f63377930648ffb6
2024/05/17 09:05:17 Loaded repository configuration file: test/data/repositories/centos-8.json
...
panic: failed to open config map "test/config-map.json": open test/config-map.json: no such file or directory

@mvo5 mvo5 closed this May 17, 2024
@achilleas-k
Copy link
Member

I'd still like to remove the command here. Maybe we can update the docs to instruct people to clone osbuild/images and run the command from there, because the configs are also needed.

We should also delete test/data/manifests/ while we're at it, but I think some tests look for them so maybe we should look at that separately.

@mvo5 mvo5 reopened this May 17, 2024
This is a result of the outcome of
osbuild#4124

Achilleas suggested that instead of having a copy of `gen-manifests`
in osbuild-composer we could use the one from images. AFAICT nothing
in the code references `gen-manifests` except the `test/README` so
it seems safe to remove and just use images.

The README got updated to point to `images` and we will also
need to remove `test/data/manifest` at a later point (after
checking that nothing else depends on this anymore).
@mvo5 mvo5 force-pushed the use-images-gen-manifests branch from 79a03a0 to 9eb3e82 Compare May 17, 2024 14:20
@mvo5 mvo5 requested a review from atodorov May 17, 2024 14:21
@mvo5
Copy link
Contributor Author

mvo5 commented May 17, 2024

I'd still like to remove the command here. Maybe we can update the docs to instruct people to clone osbuild/images and run the command from there, because the configs are also needed.

We should also delete test/data/manifests/ while we're at it, but I think some tests look for them so maybe we should look at that separately.

Thanks! I reworked the README changes based on your feedback and reopened. I also added Alexander as the last two commits in test/data/manifests/ were done by him (just to be sure that everyone involved is notified :)

@atodorov
Copy link
Contributor

We should also delete test/data/manifests/ while we're at it, but I think some tests look for them so maybe we should look at that separately.

Let me dig around the various repositories and test suites a bit more to remind myself what was there. I remember we analyzed this at the beginning of the year, removed some test scenarios which were duplicate b/w osbuild-composer and images repositories and kept the rest (there is a ticket with more details, I'll find it and we can talk it through on Monday).

My only concern ATM is that if we happen to need to update test/data/manifests/, especially b/c we'll be adding new major/minor versions to the test matrix very soon, and the tools in the 2 repos are incompatible how we'll be able to update the manifests. Sounds like a test blocking situation could happen very easily so we may have to hold off on merging this PR for some time.

I'll get back to you with more details.

in `tools/test-case-generators/repos.json`.
Those are legacy now and all manifest testing is happening in
github.com/osbuild/images - there is a similar `test/data/manifest` directory
in this repo.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see #3967 and the corresponding COMPOSER-2127 and COMPOSER-2122.

From what I am reading on the above 2 tickets + 1 PR there are still multiple image types which aren't covered by osbuild/images and it is unclear (as of now) when they will be fully covered.

Based on this we've removed duplicate test scenarios and kept ones which were only covered in the osbuild-composer repository. Based on this and the fact that the format between the 2 repositories has changed I cannot approve this PR.

Let's discuss it more before introducing any more changes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this. I admit, I lost track of how the manifests in osbuild-composer were being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Based on this I will close this PR for now until the tests are really fully transferred into a single repository.

@mvo5 mvo5 closed this May 29, 2024
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.

3 participants