cosalib/meta.py: support sub schemas #1084

darkmuggle · 2020-01-29T18:41:10Z

This adds support for discerning whether or not a sub-schema should
exist based on the name in 'meta.json' to support variants.

The new behavior to treat the main schema file as a basename. Any schema
that is found in the same directory with the basename is loaded. The
"name" field is then used to find any sub schemas. For example, given
"v1.json" with a build name of "fedora-coreos", a schema named
"fedora-coreos-v1.json" in the same directory will be applied.

fixtures/fcos.json: defined test JSON for Fedora CoreOS

src/schema/{fedora-coreos,rhcos}-v1.json: stub out for the different
schemas. This is to support the FCOS use of:
"fedora-coreos.parent-version" and
"fedora-coreos.parent-commit" keys.

tests/test_cosalib_meta.py: support testing both FCOS and RHCOS schemas

looks for all schemas in fixtures directory
validated the required items against both.

Makefile: set the directory for the test JSON.

darkmuggle · 2020-01-29T18:44:05Z

In #1062 @jlebon asked for two "fedora-coreos" keys. This addresses that need while giving both Fedora CoreOS and RHCOS the ability to define "schema" extensions.

jlebon · 2020-01-29T19:22:25Z

src/cosalib/meta.py

+        for k, v in self._validator.items():
+            if k in ("main", self.get("name")):
+                log.debug(f"validating against {k} schema")
+                v.validate(dict(self))


Hmm, so this is just doing a sequential check of the various schemas, right? But I think one other really valuable part of validating against a schema is erroring/warning against unknown keys. Otherwise, e.g. it would not have prevented #1045 really.

Briefly looking through https://python-jsonschema.readthedocs.io/en/stable/, I couldn't find an option to specify multiple schemas to "overlay", which I think is understandable.

How about something like this:

we read in the OS-specific schema file, and add the properties items to the properties of the base schema

we turn on additionalProperties: false in the base schema

Take another look :)

darkmuggle · 2020-01-30T00:49:57Z

I need to fix the tests, but the latest push:

uses a strict schema (no more extra's allowed)
uses an enum to determine what is required.

darkmuggle · 2020-01-30T02:10:36Z

@jlebon can I bother you for another review? I think this accomplishes what you suggested, while doing strict schema enforcement.

darkmuggle · 2020-01-30T13:24:51Z

I need to fix the tests, but the latest push:

uses a strict schema (no more extra's allowed)

uses an enum to determine what is required.

Fixed up. The Schema is now:

only allows "fedora-coreos" or "rhcos" as the name
based on the name, it uses "anyOf" to enumerate the required elements.

I dropped the sub-schemas and the code around ingesting them

jlebon

Yeah, I think this is a good next step for sure! 👍

I feel a little uneasy about having to repeat the common properties between rhcos and fedora-coreos, but I think we can fix that up in a follow-up by defining them as a top-level named schema and then extending the RHCOS/FCOS-specific schemas using allOf within the anyOfs? (I'm looking at https://json-schema.org/understanding-json-schema/structuring.html#reuse and https://json-schema.org/understanding-json-schema/structuring.html#extending).

Leaving it open for a bit in case anyone else wants to take a look.

arithx

Seems sane to me

arithx · 2020-01-30T16:54:42Z

src/schema/v1.json

+           "fedora-coreos.parent-commit",
+           "fedora-coreos.parent-version",
+           "images",
+           "name",


Can't name be dropped from the anyOf of both rhcos & fedora-coreos because it's in the outer level required (line 8)?

cgwalters · 2020-01-30T19:02:14Z

src/schema/v1.json

      ],
-     "pattern":"^(.*)$"
+     "enum": [


It looks like this is exhaustive? The whole idea is to support people doing custom builds, let's not force name to be one of those.

Changed to support hack builds by defining a "core" schema with the expected outputs for RHCOS and FCOS.

jlebon · 2020-01-31T18:15:49Z

Looks like this needs a rebase now.

jlebon · 2020-01-31T19:02:22Z

@petr-muller Any idea why the bot is being spammy here?

darkmuggle · 2020-02-03T18:05:10Z

@cgwalters @arithx can you gentleman take another look? The schema is conditionalized so that unsupported builds (e.g. non-CoreOS) won't be broken.

arithx · 2020-02-03T18:10:09Z

Don't know if it's just github showing the commits wrong but all of the content is in the first commit (OWNERS: add OWNERS file for PROW integration)

arithx

LGTM

arithx · 2020-02-03T18:14:45Z

src/schema/v1.json

+     "examples": [
+       "fedora/x86_64/coreos/testing-devel"
+      ],
+     "pattern":"^(?!\\s*$).+"


Optional: single quoting regex patterns to avoid having to escape backslashes can be nice

cgwalters · 2020-02-03T18:45:07Z

src/schema/v1.json

+    {
+         "if": {
+             "properties": {
+                 "name": { "pattern": "^(rhcos|fedora-coreos)"  }


Many of these are core properties emitted by coreos-assembler; I don't see why e.g. if someone is doing a custom build they shouldn't have coreos-assembler.basearch.

cgwalters · 2020-02-03T18:46:38Z

src/schema/v1.json

+                   "coreos-assembler.config-dirty",
+                   "coreos-assembler.config-gitrev",
+                   "coreos-assembler.container-config-git",
+                   "coreos-assembler.container-image-git",


Let's not make these required again please? I don't see a connection between "custom coreos-like builds" and "custom coreos-assembler builds inside an existing development container".

IOW, I want to be able to test patches to coreos-assembler and rpm-ostree inside my dev container, targeting all of fcos/rhcos and e.g. FCOSB.

The problem is that they are required for RHCOS and the purpose of the schema is to validate the results from builds. I am sympathetic towards your pain @cgwalters -- I am -- but your argument is a required production schema should be weakened for you as a developer based on your workflow.

#1089 was targeted at helping you out with this.

But the larger problem, IMO, is that we have developers who run their own pet container, while COSA expects certain things; that's the tension here. I'm open to ideas

I think delivering coreos-assembler as a container image has been very successful; I often tell people about that.

BUT I don't think we should entirely rule out e.g. shipping it as an RPM too.

And conceptually, these keys only cover the container build path.

And further, they duplicate metadata already available in the container metadata (OpenShift builds e.g. inject the git commit); unfortunately quay.io's build system is more primitive.

So I'm not sure I'd agree these keys are "required for RHCOS".

I guess I can just set the environment variable for now, it'll just be annoying.

I'll also chime in as a developer who hacks on cosa from his pet container. 🐕 I think it's important to cover that use-case. Not just for rpm-ostree/$util, but just overall I think it yields a faster edit-build-test feedback loop which makes hacking more pleasant. I've documented it in the README too.

Dropped in the last push.

After(if) this merged I'll through up one that allows for per-recipe overrides.

This changes the v1.json schema is be aware of "fedora-coreos" and "rhcos" builds. Using the "name" key, the schema is conditionally applied. fixtures/fcos.json: defined test JSON for Fedora CoreOS tests/test_cosalib_meta.py: support testing both FCOS and RHCOS schemas Makefile: set the directory for the test JSON.

cgwalters · 2020-02-03T21:52:39Z

The other aspect to this debate is; maybe we want to drive down into coreos-assembler the concept of an "official" build; we can be more strict in that case. So it'd be e.g. the pipelines which do cosa build --official or something and that adds in the config git schemas rather than triggering off the name.

cgwalters · 2020-02-03T21:54:46Z

src/schema/v1.json

+                   "coreos-assembler.config-gitrev",
+                   "coreos-assembler.image-config-checksum",
+                   "coreos-assembler.image-genver",
+                   "coreos-assembler.image-input-checksum"


I think it's OK for all of these to be required too?

Unless we're trying to be compatible with older builds (whole rat's nest there)?

I was going for the lowest common denominator here. The new schema is strict enough that having these as optional should be good enough IMHO.

cgwalters

One optional comment, otherwise LGTM

openshift-ci-robot · 2020-02-03T21:55:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arithx, cgwalters, darkmuggle, jlebon

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~~ [arithx,cgwalters,darkmuggle,jlebon]

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

darkmuggle · 2020-02-03T22:02:29Z

The other aspect to this debate is; maybe we want to drive down into coreos-assembler the concept of an "official" build; we can be more strict in that case. So it'd be e.g. the pipelines which do cosa build --official or something and that adds in the config git schemas rather than triggering off the name.

I'll look into it.

kola: Change distro default to fcos, drop cl

jlebon reviewed Jan 29, 2020

View reviewed changes

jlebon approved these changes Jan 30, 2020

View reviewed changes

arithx approved these changes Jan 30, 2020

View reviewed changes

cgwalters reviewed Jan 30, 2020

View reviewed changes

jlebon mentioned this pull request Jan 30, 2020

schema: Unmark container-config* keys as required #1087

Merged

jlebon added the needs-rebase label Jan 31, 2020

OWNERS: add OWNERS file for PROW integration

a34cbae

arithx approved these changes Feb 3, 2020

View reviewed changes

cgwalters reviewed Feb 3, 2020

View reviewed changes

cgwalters mentioned this pull request Feb 3, 2020

Dockerfile: Inject a .built-via-container stamp file #1096

Closed

cgwalters mentioned this pull request Feb 3, 2020

cmdlib: Add logic for default meta.json schema or skipping validation #1089

Merged

cgwalters reviewed Feb 3, 2020

View reviewed changes

cgwalters approved these changes Feb 3, 2020

View reviewed changes

darkmuggle merged commit b19cdea into coreos:master Feb 3, 2020

darkmuggle deleted the feature/sub-schemas branch February 18, 2020 21:48

jcajka pushed a commit to jcajka/coreos-assembler that referenced this pull request Mar 24, 2020

Merge pull request coreos#1084 from cgwalters/fcos-default

b844975

kola: Change distro default to fcos, drop cl

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cosalib/meta.py: support sub schemas #1084

cosalib/meta.py: support sub schemas #1084

darkmuggle commented Jan 29, 2020

darkmuggle commented Jan 29, 2020

jlebon Jan 29, 2020

darkmuggle Jan 30, 2020

darkmuggle commented Jan 30, 2020

darkmuggle commented Jan 30, 2020

darkmuggle commented Jan 30, 2020

jlebon left a comment

arithx left a comment

arithx Jan 30, 2020

cgwalters Jan 30, 2020

darkmuggle Feb 3, 2020

jlebon commented Jan 31, 2020

jlebon commented Jan 31, 2020

darkmuggle commented Feb 3, 2020

arithx commented Feb 3, 2020

arithx left a comment

arithx Feb 3, 2020

cgwalters Feb 3, 2020

cgwalters Feb 3, 2020

darkmuggle Feb 3, 2020

cgwalters Feb 3, 2020

jlebon Feb 3, 2020

darkmuggle Feb 3, 2020

cgwalters commented Feb 3, 2020

cgwalters Feb 3, 2020

darkmuggle Feb 3, 2020

cgwalters left a comment

openshift-ci-robot commented Feb 3, 2020

darkmuggle commented Feb 3, 2020

cosalib/meta.py: support sub schemas #1084

cosalib/meta.py: support sub schemas #1084

Conversation

darkmuggle commented Jan 29, 2020

darkmuggle commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkmuggle commented Jan 30, 2020

darkmuggle commented Jan 30, 2020

darkmuggle commented Jan 30, 2020

jlebon left a comment

Choose a reason for hiding this comment

arithx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Jan 31, 2020

jlebon commented Jan 31, 2020

darkmuggle commented Feb 3, 2020

arithx commented Feb 3, 2020

arithx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 3, 2020

darkmuggle commented Feb 3, 2020