-
Notifications
You must be signed in to change notification settings - Fork 7
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
docs: update maas-anvil documentation #77
Conversation
@skatsaounis Do you think we should wait with merging this PR until we have updated the help messages for the commands so they are the same as the CLI Interface section in these docs? |
@dgtlntv I have added Bill as a reviewer. What changes are we talking about? #72, #74? Both of them were resolved with the PRs that got merged today. Unless there is something more than that |
@skatsaounis In the docs in the PR under "CLI interface" I changed some of the descriptions of the commands, options. They are different from what would currently be displayed when you type --help for a command. I thought that we could update the help pages of the commands as well to the improved version outlined in the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good with some minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've changed the manifest a bit with TLS passthrough being merged today, so I added some suggestions to make things consistent here and add some more explanation
@skatsaounis As discussed I have update the PR to also include updates to the help pages in the CLI. I had to do some custom formatting for the Commands/Groups to get the desired format of the help pages. Specifically click does not seem to have a native way to group commands in the help page and indents epilog text by default. |
Updated the help pages. Also updated the readme reference section to reflect the updates.
Co-authored-by: Wyatt Rees <wyatt.rees@gmail.com>
Co-authored-by: Wyatt Rees <wyatt.rees@gmail.com>
e43e9c8
to
d49eae6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgtlntv I rebased to current main and I also fixed all linting issues.
I have only a request for you. It seems that you are using the Markdown header levels as a convenient way to set the font size. Could you please fix them so that Only one #
exists and also every child of a parent header is at most one level greater. For example, #
can have only ##
children, instead of #####
or any other level. This is the only linting issue that remains.
Finally, could you please get the produced snap, install it locally and confirm that the help text is displayed as expected?
As far as coding is concerned, I am happy to give my +1 when you take care of the above things. For the docs content, you may still chase an approval from @billwear
Thank you for your great work so far 🚀
@skatsaounis I have addressed your change requests and reviewed all help pages with a locally built snap. It looks good from my side now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1 nice job. Please get also approval from @billwear or acknowledge that we can merge without it. Afterwards, we will merge this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are all docstrings. there is no canonical standard for these, so nothing to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Updates the documentation in the README file. To see a rendered version of the changed markdown see the README in my fork.
Related Jira epic