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 documentation for Create Porter's Configuration file under Task #2045

Merged
merged 3 commits into from
May 18, 2022

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Apr 29, 2022

What does this change

What issue does it fix

Work for #1027

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Show resolved Hide resolved
Copy link
Member

@carolynvs carolynvs 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! I just have general editing feedback.

docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
- operators/configuration/
---

Porter's default behavior, such as log level, default plugins, etc., can be modified through its config file.
Copy link
Member

Choose a reason for hiding this comment

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

nit: This simplifies the punctuation in the sentence a bit. It's a good idea for us to avoid Latin abbreviations such as "e.g.", "i.e.", and "etc". Using the equivalent English words makes the content more accessible to people who speak other languages.

Suggested change
Porter's default behavior, such as log level, default plugins, etc., can be modified through its config file.
Porter's default behavior, such as the log level and default plugins, can be modified through its config file.

Copy link
Member

Choose a reason for hiding this comment

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

Marking as unresolved since the change wasn't accepted or replied to.

docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
@carolynvs
Copy link
Member

@VinozzZ I'm seeing comments marked as resolved but no corresponding change in the pushed commits. Can you please check if you have uncommitted changes or some commits that haven't been pushed yet?

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ
Copy link
Contributor Author

VinozzZ commented May 5, 2022

@VinozzZ I'm seeing comments marked as resolved but no corresponding change in the pushed commits. Can you please check if you have uncommitted changes or some commits that haven't been pushed yet?

Oops, you are right. I forgot to push up some changes. Should be fixed now

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Your new page isn't showing up in the site navigation. If you add a link to this file, https://github.com/getporter/porter/blob/release/v1/docs/config.toml then it will show up.

Don't forget that you can preview the docs locally too, which can be quicker than relying on the netlify previews: https://github.com/getporter/porter/blob/main/CONTRIBUTING.md#preview-documentation

docs/content/plugins/filesystem.md Outdated Show resolved Hide resolved
docs/content/plugins/filesystem.md Outdated Show resolved Hide resolved
docs/content/plugins/filesystem.md Outdated Show resolved Hide resolved

First check if you have PORTER_HOME set on your machine:
```console
$ echo $PORTER_HOME
Copy link
Member

Choose a reason for hiding this comment

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

The porter documentation should always show commands that work in any shell (like porter commands) or we need to provide an example for each shell (Bash + Powershell).

I think the docs here could be redone pretty easily to avoid "bash-isms" by not setting the PORTER_HOME env var (which I honestly don't recommend anyone should set anyway unless they really need to change the home directory which we aren't doing here). Then you can reference were to locate the config file with "in your PORTER_HOME directory, which defaults to ~/.porter" and leave it up to them to find the directory.

Copy link
Member

Choose a reason for hiding this comment

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

The most recent changes don't quite address what was called out here. I'll add some specific comments on the docs to give examples of how to achieve what we need.

docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

I ran into trouble replying to resolved conversations, so may end up duplicating comments on the most recent versions of files to make sure they show up. 🤷‍♀️


First check if you have PORTER_HOME set on your machine:
```console
$ echo $PORTER_HOME
Copy link
Member

Choose a reason for hiding this comment

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

The most recent changes don't quite address what was called out here. I'll add some specific comments on the docs to give examples of how to achieve what we need.

- operators/configuration/
---

Porter's default behavior, such as log level, default plugins, etc., can be modified through its config file.
Copy link
Member

Choose a reason for hiding this comment

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

Marking as unresolved since the change wasn't accepted or replied to.

docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
docs/content/end-users/configuration.md Outdated Show resolved Hide resolved
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Copy link
Member

@carolynvs carolynvs 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, thanks for your patience while we worked out the editing! 🚀

@carolynvs carolynvs merged commit f9e0476 into getporter:release/v1 May 18, 2022
Porter and Mixins automation moved this from In Progress to Done May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants