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

Add validation for paths in config #166

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

wjt
Copy link
Member

@wjt wjt commented Jun 21, 2024

Previously, if a filename referred to in the configuration does not exist, that would only be discovered when the hook which uses that key runs, which may be minutes or hours into the build.

Add a way to specify in the schema that certain keys must be set to a single path to an extant file, or a list of paths to extant files.

Add key_type = path(s) annotations to the schema for all (I think) of the file-like keys used by hooks in this repo.

https://phabricator.endlessm.com/T35517

@wjt
Copy link
Member Author

wjt commented Jun 21, 2024

I should be clear that this is only very lightly tested. I aimed to spend no more time on it than the image builder spent on a doomed image build overnight :)

@wjt wjt marked this pull request as draft June 21, 2024 14:09
@wjt
Copy link
Member Author

wjt commented Jun 21, 2024

So lightly tested that it fails its tests!

@wjt wjt force-pushed the T35517-validate-paths-in-config branch from 47d111e to 0775b73 Compare June 21, 2024 15:56
@wjt
Copy link
Member Author

wjt commented Jun 21, 2024

Oh I guess I can't use match unless we bump our minimum Python version to 3.10 (i.e. Endless OS 6 is required).

@dbnicholson I'm curious what you think about this approach, before I mess with going back to if/elif etc.

Of course this wouldn't catch the error at PR time – I think the way to do that would be to have a list of known combinations of fields and have a GitHub Actions job which validates the config for all those. (And a similar job in the private config repo which is where the error occurred.) My old idea there is to again abuse the schema. Add the following to the config:

[build]
configtuple = ${product}-${arch}-${platform}-${personality}

and then add to the schema:

[build]
configtuple_values = eos-amd64-amd64-base
                     eos-amd64-amd64-en
                     # ... etc

@dbnicholson
Copy link
Member

Oh I guess I can't use match unless we bump our minimum Python version to 3.10 (i.e. Endless OS 6 is required).

@dbnicholson I'm curious what you think about this approach, before I mess with going back to if/elif etc.

Sorry, I meant to reply to this long ago. I think it's a nice idea, and I would switch away from match since the intention of this builder is that you can run it on basically any host that has python3 and ostree. Limiting that to only Python >= 3.10 would be a little limiting, although less so as time marches on. You wouldn't be able to run it on Debian bullseye (oldstable) or Ubuntu focal (20.04LTS).

Of course this wouldn't catch the error at PR time – I think the way to do that would be to have a list of known combinations of fields and have a GitHub Actions job which validates the config for all those. (And a similar job in the private config repo which is where the error occurred.) My old idea there is to again abuse the schema. Add the following to the config:

[build]
configtuple = ${product}-${arch}-${platform}-${personality}

and then add to the schema:

[build]
configtuple_values = eos-amd64-amd64-base
                     eos-amd64-amd64-en
                     # ... etc

Generally I like this idea, but we'd only be able to manage the allowed values in our private config repo. It would be artificially limiting to 3rd parties to constrain those fields in this repo. Oh, it does look like I added the ability to read the schema from the local directory in 3e03f8a, so that's good. Unfortunately, it doesn't merge values, so any schema value we put in this repo would have to be duplicated.

It might be nice to be able to use wildcards to you could have an allowed value like eoscustom-amd64-amd64-*, but that's probably a whole can of worms. For our purposes, a strict list of allowed values in our private repo would be enough.

@wjt
Copy link
Member Author

wjt commented Nov 25, 2024

I think wildcards would be useful even in the private repo. We have often made an identical copy of an existing personality with a project-specific suffix just to give it a distinctive name for our activation/metrics data. With the changes on #169 we'd no longer need to change anything in the repo to build an identical-in-all-but-name derived image. It would be sad to have to update the schema each time...

I'll see if I can find time this week to revive this one.

@wjt wjt force-pushed the T35517-validate-paths-in-config branch 2 times, most recently from 2e470f3 to ec72cbb Compare November 26, 2024 15:08
wjt added 2 commits November 26, 2024 15:23
Previously, if a filename referred to in the configuration does not
exist, that would only be discovered when the hook which uses that key
runs, which may be minutes or hours into the build.

Add a way to specify in the schema that certain keys must be set to a
single path to an extant file, or a list of paths to extant files.

https://phabricator.endlessm.com/T35517
Based on a quick review of the hooks in this repo, I think this covers
all the likely candidates.

https://phabricator.endlessm.com/T35517
@wjt wjt force-pushed the T35517-validate-paths-in-config branch from ec72cbb to 8f8d720 Compare November 26, 2024 15:23
@wjt wjt marked this pull request as ready for review November 26, 2024 15:29
@wjt
Copy link
Member Author

wjt commented Nov 26, 2024

Revised this to use more traditional Python syntax and add a test. I didn't add any of the config tuple validation stuff described above.

Copy link
Member

@dbnicholson dbnicholson left a comment

Choose a reason for hiding this comment

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

👍

@dbnicholson dbnicholson merged commit ec57796 into master Nov 26, 2024
2 checks passed
@dbnicholson dbnicholson deleted the T35517-validate-paths-in-config branch November 26, 2024 22:58
wjt added a commit that referenced this pull request Nov 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.

2 participants