Expand config validation to most of the config #73

shanejbrown · 2023-09-12T21:56:13Z

Description

Expanded configuration file validation to most of the available fields. This has the potential of breaking existing builds which are using buildrunner in a non-supported way, contrary to the written buildrunner documentation. This will fail immediately if the configuration is invalid for any reason.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

bluesliverx · 2023-09-13T16:15:03Z

buildrunner/validation/config.py

+
+    class SSHKey(BaseModel, extra='forbid'):
+        """ SSH key model """
+        file: Optional[str]


Are these working without assigning "None" to the value? That's surprising to me, I thought newer pydantic versions required it

Yes it appears to be working. But I don't quite understand what you are saying. Can you give me an example of what you mean?

I've had to do file: Optional[str] = None to get it working in some cases (newer pydantic versions I thought)

This is using an older version of pydantic version 1.10.11 because that is the highest that python-on-whales allows currently. That may be why I am able to get away with using that syntax.

ah, yes, I knew that

bluesliverx · 2023-09-13T17:25:11Z

buildrunner/validation/config.py

@@ -188,5 +157,5 @@ def validate_config(**kwargs) -> Errors:
    try:
        Config(**kwargs)
    except ValidationError as exc:
-        errors = _add_validation_errors(exc)
+        errors = add_validation_errors(exc)


Is this really adding validation errors or just getting validation errors?

Yes, you are correct. It really just gets the errors. I'll make the change.

bluesliverx · 2023-09-13T17:26:13Z

buildrunner/validation/step.py

+from pydantic import BaseModel, Field
+
+
+class StepPypiPush(BaseModel):


Should this forbid extra?

I can do that. I just wasn't sure if the documentation was complete on that so I erred on the side of having it allow more. I'll add that in and hope for the best. :)

bluesliverx · 2023-09-13T17:26:44Z

buildrunner/validation/config.py



 class Config(BaseModel):
    """ Top level config model """
+
+    # Unclear if this is actively used
+    class GithubModel(BaseModel, extra='forbid'):


This is cool syntax, didn't know that was possible. Should the Config top-level class also forbid extra?

I was just afraid that I may have missed something. I'll add it in to the top-level config. Then if it fails we can address where the issue is, either update the validation code, the documentation or the buildrunner config with the issue.

bluesliverx · 2023-09-13T17:27:25Z

buildrunner/validation/step.py

+    """ Run model within a step """
+
+    xfail: Optional[bool]
+    services: Optional[Dict[str, Any]]


I feel like this should be Optional[Dict[str, Service]] or something so that the services get validated as well.

It might share a lot with run as well, so maybe there can be a base class that is shared between them?

I made this update, to validate the Service portion.

bluesliverx · 2023-09-13T17:28:35Z

buildrunner/validation/step.py

+    ports: Optional[Dict[str, str]]
+    volumes_from: Optional[List[str]]
+    ssh_keys: Optional[List[str]] = Field(alias='ssh-keys')
+    artifacts: Optional[Dict[str, Union[Artifact, None]]]


Isn't this Optional[Dict[str, Optional[Artifact]]]? i.e. Optional instead of the Union with None

Yes. I like that. It is more concise. I'll make the update.

bluesliverx · 2023-09-13T17:29:46Z

buildrunner/validation/step.py

+    pull: Optional[bool]
+    platform: Optional[str]
+    platforms: Optional[List[str]]
+    inject: Optional[Dict[str, str]]


It might be possible to use inject with a key but no value, and it defaults to the working dir, but I could be remembering wrong here. It might be worth checking inject in various buildrunner files in git.

I couldn't find a buildrunner file which had a inject value as None. But I am ok with making this check less strict. I'll make the value Optional.

bluesliverx · 2023-09-13T17:31:11Z

tests/test_validation/test_validation_config.py

+    config_yaml = """
+    version: 2.0
+    steps:
+      my-build-step:


Nice that you're testing the docs, but maybe this should be a separate file that we can then load into the docs as well directly?

That is a great idea. Are you thinking that the README.rst would just have a link to the example file?.

I would say it's not an exact copy and paste from the README I had to make some edits to make it valid YAML.

shanejbrown force-pushed the full-validation branch 3 times, most recently from 0c32f3d to cafa374 Compare September 13, 2023 15:56

shanejbrown added 2 commits September 13, 2023 09:57

Expand config validation to most of the config

d1ec809

Loosen strictness of config validation

6ddbe51

shanejbrown force-pushed the full-validation branch from cafa374 to 6ddbe51 Compare September 13, 2023 15:58

shanejbrown marked this pull request as ready for review September 13, 2023 16:10

bluesliverx reviewed Sep 13, 2023

View reviewed changes

Clean up and add Service validation model

9948d56

bluesliverx approved these changes Sep 13, 2023

View reviewed changes

shanejbrown merged commit 510e77a into adobe:main Sep 14, 2023
8 checks passed

shanejbrown deleted the full-validation branch September 19, 2023 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand config validation to most of the config #73

Expand config validation to most of the config #73

shanejbrown commented Sep 12, 2023 •

edited

Loading

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

bluesliverx Sep 13, 2023

shanejbrown Sep 13, 2023

shanejbrown Sep 13, 2023

		from pydantic import BaseModel, Field


		class StepPypiPush(BaseModel):

Expand config validation to most of the config #73

Expand config validation to most of the config #73

Conversation

shanejbrown commented Sep 12, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

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

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

shanejbrown commented Sep 12, 2023 •

edited

Loading