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

unrecognized key warning #191

Merged
merged 6 commits into from
Mar 25, 2020
Merged

unrecognized key warning #191

merged 6 commits into from
Mar 25, 2020

Conversation

ben-bay
Copy link
Contributor

@ben-bay ben-bay commented Mar 24, 2020

This will save users (and me, frankly) lots of headache due to slight misspellings or indentation errors.

Merlin still allows extra keys; but since they are never used, it will be helpful to show a warning whenever one is found.

Have I missed any keys that merlin supports?

@ben-bay ben-bay requested a review from koning March 24, 2020 04:05
merlin/spec/all_keys.py Show resolved Hide resolved
@koning
Copy link
Member

koning commented Mar 24, 2020

We should also add env/labels to the spec.

@ben-bay
Copy link
Contributor Author

ben-bay commented Mar 24, 2020

We should also add env/labels to the spec.

That's on line 48 currently

@koning
Copy link
Member

koning commented Mar 24, 2020

We should also add env/labels to the spec.

That's on line 48 currently

I meant in the spec.

@ben-bay
Copy link
Contributor Author

ben-bay commented Mar 24, 2020

We should also add env/labels to the spec.

That's on line 48 currently

I meant in the spec.

Now that I look at this section in the specification doc, I think I removed the labels section in the past, at the time we were starting to discourage use of env.labels in favor of env.variables.

@FrankD412
Copy link
Member

@ben-bay -- I've looked over the PR, and it looks to me like the necessary keys are there. That said, it looks like you're improving the checking in the specification which is something I've wanted to add to Maestro myself. I'd like to collaborate with you to back port this functionality to Maestro as I think that's where the core of this functionality should reside. That would make it so that Maestro is responsible for its keys in the base YAMLSpecification while Merlin just has to check for any extra keys it's added simply by implementing its check on top of the base check.

@ben-bay
Copy link
Contributor Author

ben-bay commented Mar 25, 2020

@ben-bay -- I've looked over the PR, and it looks to me like the necessary keys are there. That said, it looks like you're improving the checking in the specification which is something I've wanted to add to Maestro myself. I'd like to collaborate with you to back port this functionality to Maestro as I think that's where the core of this functionality should reside. That would make it so that Maestro is responsible for its keys in the base YAMLSpecification while Merlin just has to check for any extra keys it's added simply by implementing its check on top of the base check.

@FrankD412 brilliant, happy to work with you on this. Glad you share my sentiment here-- you have no idea how much time the occasional warning would've saved me. I agree backporting seems like the natural solution.

@ben-bay
Copy link
Contributor Author

ben-bay commented Mar 25, 2020

@FrankD412 for now I'll merge this, then when our backport is complete I'll remove the maestro material on my end.

@ben-bay ben-bay merged commit 80db30d into develop Mar 25, 2020
@ben-bay ben-bay deleted the bugfix/ben/bad_key_warning branch March 25, 2020 17:17
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