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 config.schema.json #167

Closed
wants to merge 26 commits into from
Closed

add config.schema.json #167

wants to merge 26 commits into from

Conversation

cjyetman
Copy link
Member

No description provided.

@cjyetman cjyetman marked this pull request as draft February 25, 2024 14:54
@cjyetman
Copy link
Member Author

@AlexAxthelm this is still in draft mode, assuming you may have some suggestions. Also still need to figure out appropriate values for the "$id", "title", and "description" keys... and configure the GH Action appropriately and make sure it works as expected.

@AlexAxthelm
Copy link
Collaborator

Exciting! I'll take a closer look later!

@AlexAxthelm
Copy link
Collaborator

@cjyetman Noting a couple of things here from the docs used in the action:

First: it doesn't look like we're actually validating against the schema you wrote. From the docs:

Input Required? Default Description
yaml_schema false "" The full path to the YAML schema file (e.g. ./schemas/schema.yaml) - Default is "" which doesn't enforce a strict schema

So right now it's just doing a check on "is this a valid yaml file", not "does this yaml file match this schema".

Second: I don't think we can make a good schema that will cover the inheritance that we're counting on with config::get().

The Schema should model a fully-defined object, but as we have the files written (2023Q4_azure inherits from 2023Q4 inherits from docker inherits from default), none of those objects will necessarily have all the correct keys.

Here's an article about modeling inheritance with JSON schema: https://json-schema.org/blog/posts/modelling-inheritance but the short story is that it isn't as intuitive as config makes it.

I think this absolutely makes sense for us to chase down, but it might involve bigger changes to how we handle/import configs than just doing the check.

@AlexAxthelm
Copy link
Collaborator

One "workaround" option would be that for each of the configs, we read them in with config::get() (to deal with inheritance), and write them to file before checking them against schema.

@cjyetman
Copy link
Member Author

Yeah, I had a feeling this validator action isn't going to work with the inheritance easily. I'm definitely not wedded to it, just thought it would be cool if it worked.

@cjyetman
Copy link
Member Author

cjyetman commented Mar 1, 2024

gonna remove the validator action because I don't think there's any reasonable way of making it work as expected/desired

@cjyetman cjyetman changed the title add config.schema.json and validator action add config.schema.json Mar 7, 2024
Copy link

Docker build status

Commit time Git sha Image
2024-04-30T06:17:54Z b293b74 ghcr.io/rmi-pacta/workflow.data.preparation:pr-167
History
Commit time Git sha Image
2024-04-30T06:17:54Z b293b74 ghcr.io/rmi-pacta/workflow.data.preparation:pr-167
History JSON`[{"commit_time":"2024-04-30T06:17:54Z","git_sha":"b293b74f996b1c2c433fd086794e77cc92fcd2f5","image":"ghcr.io/rmi-pacta/workflow.data.preparation:pr-167"}]`

@cjyetman
Copy link
Member Author

While this was a cool experiment, I don't know if this would ever get used and it has already fallen out of date, which suggests to me this will be more of a maintenance burden than useful, so I'm gonna close it.

@cjyetman cjyetman closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants