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

schema: Add entry for base-oscontainer #2907

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

cgwalters
Copy link
Member

For now, we need to support having the new format oscontainer in
meta.json.

Part of #2685
And see #2685 (comment)
in particular.

@cgwalters
Copy link
Member Author

I was trying to follow in the footsteps of the kubevirt change in #2750
but I do not understand all the stuff around schemas. I see we have make schema-check
which starts out

.PHONY: schema-check
schema-check: DIGEST = $(shell sha256sum src/v1.json | awk '{print $$1}')
schema-check:
	# Are the JSON Schema copies synced with each other?
	diff -u src/v1.json schema/v1.json

And right there I have questions - like, why are there two files at all?

Then the schema is apparently copied again into both mantle and gangplank code? 🤯
This seems like it really wants #2821

@bgilbert
Copy link
Contributor

bgilbert commented Jun 8, 2022

Yeah, the multiple copies aren't great. The test is there because our current architecture requires them, and we kept breaking things by getting the copies out of sync.

You'll need to revendor both gangplank and mantle.

@cgwalters
Copy link
Member Author

The test is there because our current architecture requires them

Comments on #2821 appreciated 😄

You'll need to revendor both gangplank and mantle.

Ahhh! Now I see the

replace (
	github.com/coreos/coreos-assembler-schema => ../schema

bit and I understand how this works.

@cgwalters cgwalters force-pushed the schema-ostreecontainer branch 2 times, most recently from 7a922fb to 4cf0fe4 Compare June 8, 2022 19:59
@cgwalters
Copy link
Member Author

OK, tested this fully now.

@cgwalters
Copy link
Member Author

/override ci/prow/rhcos

@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2022

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/rhcos

In response to this:

/override ci/prow/rhcos

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.

@cgwalters
Copy link
Member Author

/override ci/prow/rhcos

@openshift-ci
Copy link

openshift-ci bot commented Jun 9, 2022

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/rhcos

In response to this:

/override ci/prow/rhcos

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.

@@ -211,6 +211,7 @@ var generatedSchemaJSON = `{
"amis",
"azure",
"azurestack",
"baseos-container",
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: should this be base-oscontainer instead for consistency with the existing oscontainer?

Or maybe native-oscontainer to better highlight the difference between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK renamed to base-oscontainer.

For now, we need to support having the new format oscontainer in
`meta.json`.

Part of coreos#2685
And see coreos#2685 (comment)
in particular.
While I am trying to actively sever the dependence of the base
container image build on `meta.json`, there's no reason not
to inject it into `meta.json` in this flow too because the build
system already requires it.
@cgwalters
Copy link
Member Author

Note we should change the FCOS pipeline to use this here https://github.com/coreos/fedora-coreos-pipeline/blob/515ae3806f70dc3ede18c9a885bade2d0cce3d2f/jobs/release.Jenkinsfile#L132

Also, this is going require changing the RHCOS pipeline to also do:

  • push to S3
  • push container
  • push to S3

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.

None yet

3 participants