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

docs: update maas-anvil documentation #77

Merged
merged 37 commits into from
Dec 12, 2024
Merged

Conversation

dgtlntv
Copy link
Member

@dgtlntv dgtlntv commented Nov 13, 2024

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

@dgtlntv
Copy link
Member Author

dgtlntv commented Nov 13, 2024

@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?
Also could you maybe add @billwear as a reviewer to this PR? I am not able to add reviewers myself.

@skatsaounis skatsaounis requested a review from billwear November 13, 2024 12:05
@skatsaounis
Copy link
Collaborator

skatsaounis commented Nov 13, 2024

@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? Also could you maybe add @billwear as a reviewer to this PR? I am not able to add reviewers myself.

@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 skatsaounis changed the title Update the documentation docs: update maas-anvil documentation Nov 13, 2024
@dgtlntv
Copy link
Member Author

dgtlntv commented Nov 13, 2024

@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.

Copy link
Contributor

@wyattrees wyattrees left a 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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@wyattrees wyattrees left a comment

Choose a reason for hiding this comment

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

one last update

Copy link
Contributor

@wyattrees wyattrees left a 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

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dgtlntv
Copy link
Member Author

dgtlntv commented Nov 19, 2024

@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.
Do you think it is ok to add this to get nicer looking help pages? If yes could you help me resolve the linter issues? I am not too familiar with coding with types in python 😅

@skatsaounis skatsaounis force-pushed the main branch 3 times, most recently from e43e9c8 to d49eae6 Compare November 27, 2024 14:15
Copy link
Collaborator

@skatsaounis skatsaounis left a 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 🚀

@dgtlntv
Copy link
Member Author

dgtlntv commented Dec 4, 2024

@skatsaounis I have addressed your change requests and reviewed all help pages with a locally built snap. It looks good from my side now!

Copy link
Collaborator

@skatsaounis skatsaounis left a 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

@skatsaounis skatsaounis requested a review from wyattrees December 5, 2024 11:00
Copy link

@billwear billwear left a 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.

Copy link
Contributor

@wyattrees wyattrees left a comment

Choose a reason for hiding this comment

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

LGTM!

@skatsaounis skatsaounis merged commit 3687975 into canonical:main Dec 12, 2024
5 checks passed
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