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

Update live reference docs #1902

Merged
merged 3 commits into from
May 6, 2021
Merged

Update live reference docs #1902

merged 3 commits into from
May 6, 2021

Conversation

mortent
Copy link
Contributor

@mortent mortent commented May 5, 2021

This covers the kpt live commands.

@mortent mortent requested a review from frankfarzan as a code owner May 5, 2021 02:54
Copy link
Contributor

@frankfarzan frankfarzan left a comment

Choose a reason for hiding this comment

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

Some of the comments apply to commands in pkg as well.

site/reference/live/README.md Outdated Show resolved Hide resolved
### Synopsis
<!--mdtogo:Long-->
```
kpt live apply DIR [flags]
kpt live apply [DIR|-] [flags]
Copy link
Contributor

Choose a reason for hiding this comment

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

We're using PKG_PATH for package directories. DIR for normal directories. e.g.:

https://kpt.dev/reference/pkg/update/

Same for other commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also have a consistent description for the flag. "Path to a directory containing KRM resources and a Kptfile" is a awkward (The critical thing is Kptfile, otherwise not having any other KRM resources is a valid case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the reference docs for all the live commands. I added some new language to the description, but I'm open to suggestions here.

site/reference/live/apply/README.md Outdated Show resolved Hide resolved
```
--poll-period:
The frequency with which the cluster will be polled to determine
the status of the applied resources. The default value is every 2 seconds.
the status of the applied resources. The default value is 2 seconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

General question: what's the ordering of the flag here? It should be alphabetical but it's not (Applies to other commands as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this for the live reference docs. Will do another pass at the pkg docs when we have merged this one.

@@ -1,111 +1,81 @@
---
title: "Status"
title: "`status`"
Copy link
Contributor

Choose a reason for hiding this comment

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

@phanimarupaka are we changing this command to be a verb i.e. observe? Same question for tree, there are the only two noun commands.

DIR|-:
Path to a directory containing KRM resources and a Kptfile with inventory
information. The path can be relative or absolute. Providing '-' instead
of the path to a directory cause kpt to read from stdin.
```

#### Flags
Copy link
Contributor

Choose a reason for hiding this comment

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

should the ex-global flags (your other PR) be listed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those apply to all commands in live command group, so I think those should be included in the live page. Similar, we probably want to document the global flags on the top-level group.
Alternatively we can look into ways to include subtemplates so we can get the complete set of flags for every command, but this will lead to a pretty long list of flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they apply to all live commands, then we should include them in reference/live. Is that a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this looks good otherwise, I can do it as a follow-up.

@mortent mortent requested a review from frankfarzan May 6, 2021 19:54
@mortent mortent merged commit 87c72d6 into kptdev:next May 6, 2021
frankfarzan pushed a commit to frankfarzan/kpt that referenced this pull request Jun 3, 2021
* Update live reference docs

* Addressed comments

* Addressed comments
@mortent mortent deleted the UpdateLiveRefDocs branch June 11, 2021 20:37
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.

None yet

2 participants