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 comments support for deployment.yaml #6339

Merged
merged 2 commits into from
Aug 11, 2022
Merged

Add comments support for deployment.yaml #6339

merged 2 commits into from
Aug 11, 2022

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Aug 9, 2022

Summary

Opening this as a separate PR for @cicdw evaluation. Essentially allows deployment.yaml to have inline help in the form of comments, that are provided as Field(..., yaml_comment="my comment"). For example (see work queue name):

image

Steps Taken to QA Changes

Checklist

This pull request is:

  • A documentation / typographical error fix
    • No tests or issue needed
  • A short code fix
    • Please reference the related issue by including "closes <link to issue>" in this Pull Request's summary section.
      • If no issue exists, please create a bug report issue
    • Please include tests. One-line fixes without tests will not be accepted unless it's related to the documentation only.
  • A new feature implementation
    • Please reference the related issue by including "closes <link to issue>" in this Pull Request's summary section.
      • If no issue exists, please create a feature enhancement issue
    • Please include tests
    • Please make sure that your QA steps are both thorough and easy to reproduce by somebody with limited knowledge of the feature that you are submitting

Happy engineering!

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

I like this!! I would really like us to move to smaller PRs so if you'd like to open this as an isolated PR I'd review 👍

@jlowin jlowin changed the base branch from work-queue-name to main August 9, 2022 23:18
@jlowin
Copy link
Member Author

jlowin commented Aug 9, 2022

@cicdw this is now standalone against main

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Test coverage for yaml_comment and the consequent output would be nice

@jlowin
Copy link
Member Author

jlowin commented Aug 11, 2022

If #6343 is merged, I'll add yaml_comment for the work queue name field (as in the screenshot at the top of this PR) and use that as a non-contrived test

@zanieb zanieb merged commit ab65bc0 into main Aug 11, 2022
@zanieb zanieb deleted the deployment-yaml branch August 11, 2022 18:02
@zanieb
Copy link
Contributor

zanieb commented Aug 11, 2022

This one seems simpler to move forward, we can add the test over there?

@jlowin
Copy link
Member Author

jlowin commented Aug 11, 2022

Sure

@jlowin
Copy link
Member Author

jlowin commented Aug 11, 2022

For posterity: test added here 042de3c (#6343)

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