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

yaml validation + alt implementation #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rajkrishnamurthy
Copy link
Collaborator

DO NOT MERGE unless you unit test. The current implementation uses custom html generation based on our own tag parsing. ERB provides an alternative option, and possibly cleaner. Some tags such as are still to be implemented: Implementation Guidelines, Sampling Period and SLO Recommendation.

Copy link
Collaborator Author

@rajkrishnamurthy rajkrishnamurthy left a comment

Choose a reason for hiding this comment

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

@pritikin , @apannetrat, @mosi-k-platt , @JonathanChristopherson : Please do not merge yet, unless we have a chance to discuss and you are able to test it. I should have excluded the current implementation file: metrics_validator.rb,

@pritikin
Copy link
Collaborator

I created issue #60 to further discuss if this direction is a better path for generating the html. Happy to discuss here as well but it feels like this PR isn’t ready so I’d vote to reject it either way until a clean PR is ready.

Not ready means:

  • all existing tags should be supported; we can’t just leave the generated html in an incomplete state
  • filenames should represent final targets not “alt_implementation”
  • why are there any changes in primary_dataset.yml? Why is this work combined with issue YAML Syntax Errors in Continuous Audit Metrics #57 ?

Copy link
Collaborator

@pritikin pritikin left a comment

Choose a reason for hiding this comment

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

IF we agree to this transition I think a cleaner and more complete PR could be submitted. See my main comments.

@mosi-k-platt
Copy link
Collaborator

@rajkrishnamurthy I agree with @pritikin - It seems like it would be simpler if the YAML file changes were in a separate PR.

@rajkrishnamurthy
Copy link
Collaborator Author

@pritikin @mosi-k-platt: I will create a separate PR for the YAML file. changes. This will also allow for @aj-stein-nist to validate as well. Just curious, do we know how the format got out of sync.
PS: I'm traveling today and will work on this tomorrow.

@pritikin
Copy link
Collaborator

@rajkrishnamurthy make sure you take a look at #63 to keep up to date on latest

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.

3 participants