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

feat: Add support to generate a JSON config file only for applications purposes #1089

Merged

Conversation

helensilva14
Copy link
Contributor

@helensilva14 helensilva14 commented Feb 5, 2024

Fixes #908

New optional CLI flag parameter for any type of validation:

[--config-file-json or -cj CONFIG_FILE_JSON]
JSON Config File Path to be used for storing validations for application purposes, since we only support generating a JSON config file and all other features are exclusive for YAML config files.

/gcbrun

noxfile.py Outdated Show resolved Hide resolved
tests/unit/test__main.py Outdated Show resolved Hide resolved
@helensilva14 helensilva14 force-pushed the issue901-json-config-schema-validation-excluding-cols branch from b62867a to e4e2f4a Compare February 7, 2024 20:56
@helensilva14 helensilva14 force-pushed the issue901-json-config-schema-validation-excluding-cols branch from e4e2f4a to 0d43de9 Compare February 7, 2024 21:06
@helensilva14 helensilva14 marked this pull request as ready for review February 7, 2024 23:00
@helensilva14 helensilva14 requested a review from a team as a code owner February 7, 2024 23:00
Copy link
Collaborator

@nehanene15 nehanene15 left a comment

Choose a reason for hiding this comment

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

Left a couple comments! Were the samples/tests/ files stale? Saw that those were deleted in this PR

README.md Outdated Show resolved Hide resolved
data_validation/cli_tools.py Outdated Show resolved Hide resolved

Our Cloud Run sample also expects a raw JSON config in the `'data'` variable shown
[here](https://github.com/GoogleCloudPlatform/professional-services-data-validator/tree/develop/samples/run#test-cloud-run-endpoint).
You can get the JSON content specific for your scenario by using our CLI and providing the argument to generate the JSON config file [`--config-file-json` or `-cj <filepath>.json`]. IMPORTANT: do not forget to make the necessary adjustments between JSON and Python objects, check [this link as a reference](https://python-course.eu/applications-python/json-and-python.php).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does Cloud Run/Airflow require the JSON to be converted to a Python object? I think the request is expected to be JSON... in that case we can remove the disclaimer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't necessarily require the conversion, but in both of our samples (Cloud Run, Airflow) we're directly initializing/stating the JSON content inside the Python code as a Dict object. So in these cases is necessary to perform the data types adjustments, but the user can always provide the pure JSON if they want to by using the json.loads() method or something similar.

@helensilva14
Copy link
Contributor Author

Left a couple comments! Were the samples/tests/ files stale? Saw that those were deleted in this PR

Yes, they were not being used at all and the directory name was quite misleading, as if it had sample code to create test methods using DVT or something like that.

@helensilva14 helensilva14 merged commit d463038 into develop Feb 12, 2024
5 checks passed
@helensilva14 helensilva14 deleted the issue901-json-config-schema-validation-excluding-cols branch February 12, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to generate a JSON config file
2 participants