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(MDC): Validate configs in CI #3128

Merged
merged 14 commits into from
Sep 13, 2022

Conversation

rahul-kumar-saini
Copy link
Contributor

@rahul-kumar-saini rahul-kumar-saini commented Sep 9, 2022

Overview

  • Adds an optional step in CI that validates all .yaml files located under the config files directory
  • See step "Dataset Config Validation" in some CI runs on this PR to see how it shows up

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Base: 92.81% // Head: 92.72% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (6ad239c) compared to base (89fe3a7).
Patch coverage: 3.57% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3128      +/-   ##
==========================================
- Coverage   92.81%   92.72%   -0.10%     
==========================================
  Files         677      678       +1     
  Lines       30933    30961      +28     
==========================================
- Hits        28712    28709       -3     
- Misses       2221     2252      +31     
Impacted Files Coverage Δ
snuba/validate_configs.py 0.00% <0.00%> (ø)
snuba/datasets/configuration/json_schema.py 100.00% <100.00%> (ø)
snuba/environment.py 83.33% <0.00%> (-16.67%) ⬇️
snuba/cli/consumer.py 68.62% <0.00%> (-3.93%) ⬇️
snuba/attribution/log.py 87.09% <0.00%> (+1.61%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rahul-kumar-saini rahul-kumar-saini marked this pull request as ready for review September 9, 2022 22:46
@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner September 9, 2022 22:46
Copy link
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

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

Nice, this looks good! The only thing I would change is to move the validation script into the snuba app, so we can do snuba validate-configs.

Copy link
Member

@enochtangg enochtangg left a comment

Choose a reason for hiding this comment

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

The print statements can probably be converted to click.echo to follow convention.

Something I noticed when running snuba validate-configs, the snuba CLI-command by default imports all modules within /cli, which includes the subscriptions-executor command. This command runs get_enabled_dataset_names() in a command-line argument (

type=click.Choice(get_enabled_dataset_names()),
) which runs load_configuration_data().

Therefore, when a user has an invalid config, and they run snuba validate-configs, the validation occurs in the loading of the subscriptions-executor module as opposed to snuba validate-configs itself.

@rahul-kumar-saini rahul-kumar-saini enabled auto-merge (squash) September 13, 2022 18:42
@rahul-kumar-saini rahul-kumar-saini merged commit ff82c39 into master Sep 13, 2022
@rahul-kumar-saini rahul-kumar-saini deleted the rahul/feat/config-validation-ci branch September 13, 2022 18:57
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.

4 participants