Skip to content

Update contributing tutorial#1423

Merged
carolynvs merged 4 commits intogetporter:mainfrom
carolynvs:contrib-tutorial-feedback
Jan 25, 2021
Merged

Update contributing tutorial#1423
carolynvs merged 4 commits intogetporter:mainfrom
carolynvs:contrib-tutorial-feedback

Conversation

@carolynvs
Copy link
Copy Markdown
Member

@carolynvs carolynvs commented Jan 22, 2021

What does this change

  • Explain the tests and how to use mage to run them.
  • Add details for other shells
  • Clarified how to use the local porter build
  • Link to contributing guide

https://deploy-preview-1423--porter.netlify.app/contribute/tutorial
https://deploy-preview-1423--porter.netlify.app/quickstart/

What issue does it fix

Fixes #1400

Notes for the reviewer

None

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

* Explain the tests and how to use mage to run them.
* Add details for other shells
* Clarified how to use the local porter build
* Link to contributing guide

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Move the instructions to install mage _after_ you checkout the porter
source.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
The previous example was outdated and triggered errors. The user wasn't
asked to replace the porter.yaml generated by porter create with it, but
if someone did, we don't want it to fail.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs
Copy link
Copy Markdown
Member Author

@iennae Would you like to review this?

@jeremyrickard
Copy link
Copy Markdown
Contributor

This LGTM to me.

@carolynvs carolynvs merged commit 8e03c7d into getporter:main Jan 25, 2021
@carolynvs carolynvs deleted the contrib-tutorial-feedback branch January 25, 2021 22:55
You can enable tab completion for mage as well, so that you can type
`mage t[TAB]` and it will complete it with the name of matching targets.

1. Install bash-completion if it isn't already installed with either `brew install
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this be applicable to windows folks? I didn't that far into windows development so I'm not sure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At the moment the tutorial says

We are improving our support for building Porter on Windows. In a few weeks, we will have it all working on any Windows shell. For now, if you are on Windows, please use Windows Subsystem for Linux (WSL). The rest of this tutorial assumes that you are inside your WSL distribution when installing prerequisites and executing the commands.

So bash completion is something that we can expect from people using WSL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have been in the process of making everything about our dev process work for Windows (without WSL) but it is taking time. So when I can, I document how to do things in PowerShell, and eventually we will get there! 😀

Comment thread magefile.go
func TestE2E() error {
mg.Deps(startLocalDockerRegistry)
defer stopLocalDockerRegistry()
mg.Deps(StarlDockerRegistry)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

curious about the StarlDockerRegistry name from startLocalDockerRegistry .. Should it be StarLDockerRegistry? Just confused here about the naming. I see that the function is defined below and it matches but I wasn't sure if the L stood for local or if this was something else so the camel casing seems weird to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it was meant to be StartDockerRegistry to match up with the StopDockerRegistry..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah it was a typo. I'll submit a PR to fix that! 🤦‍♀️

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Documentation: Contributing tutorial

4 participants