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 Pydantic Extra.forbid to shared base model config #820

Merged
merged 13 commits into from
Nov 3, 2023
Merged

Conversation

flaviuvadan
Copy link
Collaborator

…examples

Pull Request Checklist

Description of PR
This PR adds the Pydantic Extra.forbid configuration to the shared base model. This means users will no longer be faced with silent semantic errors as outlined in #819. The core change is the base model. The rest of the changes are related to fixes after introducing the new config. For instance, there were Hera examples that used subpath vs sub_path, which is exactly the sort of problem users have!

…examples

Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
@flaviuvadan flaviuvadan added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Oct 29, 2023
@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #820 (887726b) into main (961963b) will increase coverage by 0.1%.
Report is 1 commits behind head on main.
The diff coverage is 89.7%.

❗ Current head 887726b differs from pull request most recent head 6471686. Consider uploading reports for the commit 6471686 to get more accurate results

@@           Coverage Diff           @@
##            main    #820     +/-   ##
=======================================
+ Coverage   79.4%   79.6%   +0.1%     
=======================================
  Files         45      45             
  Lines       3689    3726     +37     
  Branches     741     754     +13     
=======================================
+ Hits        2932    2966     +34     
- Misses       560     563      +3     
  Partials     197     197             
Files Coverage Δ
src/hera/shared/_base_model.py 100.0% <100.0%> (ø)
src/hera/workflows/_mixins.py 80.0% <100.0%> (+<0.1%) ⬆️
src/hera/workflows/artifact.py 92.3% <100.0%> (ø)
src/hera/workflows/user_container.py 86.2% <86.1%> (-1.8%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -107,6 +107,7 @@ Explore the examples through the side bar!
| [template-defaults](https://github.com/argoproj/argo-workflows/blob/master/examples/template-defaults.yaml) |
| [testvolume](https://github.com/argoproj/argo-workflows/blob/master/examples/testvolume.yaml) |
| [timeouts-step](https://github.com/argoproj/argo-workflows/blob/master/examples/timeouts-step.yaml) |
| [webhdfs-input-output-artifacts](https://github.com/argoproj/argo-workflows/blob/master/examples/webhdfs-input-output-artifacts.yaml) |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI upstream example contains an extra field that is not accepted, so moved to hera examples. I looked at the OpenAPI spec on main and it does not have the field so moved until upstream is fixed

@flaviuvadan flaviuvadan changed the title add extra forbid to shared base model config, move hdfs example, fix … Add Pydantic Extra.forbit to shared base model config Oct 29, 2023
@flaviuvadan flaviuvadan changed the title Add Pydantic Extra.forbit to shared base model config Add Pydantic Extra.forbid to shared base model config Oct 29, 2023
flaviuvadan and others added 6 commits October 28, 2023 19:25
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

Looking good 💯 Only thing I want to confirm is whether errors will raise on unknown kwargs - could we add a unit test for it?

src/hera/workflows/_mixins.py Show resolved Hide resolved
src/hera/workflows/user_container.py Show resolved Hide resolved
Comment on lines +43 to +44
env: EnvT = None # type: ignore[assignment]
env_from: EnvFromT = None # type: ignore[assignment]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does the linter have an issue with assignment here? 🤔 Are the "ignores" actually still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parent class has these fields that are overwritten Screenshot 2023-11-01 at 07 15 29

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And you get this nonsense src/hera/workflows/user_container.py:43: error: Incompatible types in assignment (expression has type "Union[_BaseEnv, EnvVar, List[Union[_BaseEnv, EnvVar, Dict[str, Any]]], Dict[str, Any], None]", base class "UserContainer" defined the type as "Optional[List[EnvVar]]") [assignment]

Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

LGTM, tiny word change first though

Co-authored-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

:shipit:

@elliotgunton elliotgunton merged commit a0c9272 into main Nov 3, 2023
16 checks passed
@elliotgunton elliotgunton deleted the fv/config branch November 3, 2023 16:35
flaviuvadan added a commit that referenced this pull request Nov 28, 2023
flaviuvadan added a commit that referenced this pull request Nov 28, 2023
This reverts commit a0c9272.

Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
elliotgunton pushed a commit that referenced this pull request Nov 28, 2023
Reverts #820

---------

Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hera should error out when users set kwargs that are invalid
3 participants