Add porter mixin create command by joshuabezaleel · Pull Request #1602 · getporter/porter

joshuabezaleel · 2021-05-23T12:31:55Z

What does this change

Introduce porter mixin create command which clone the getporter/skeletor repo and modify the skeletor's file's content according to the argument and flags supplied by the user when running the command according to the skeletor's README instruction.

The structure of the command is as follows as @carolynvs mentioned here.
porter mixin create NAME --author YOURNAME [--dir /path/to/mixin/dir]

What issue does it fix

Closes #1512

Notes for the reviewer

I attached a short video demo of running the command below.

porter.mixin.create.mp4

Checklist

Unit Tests
Documentation
Schema (porter.yaml)

joshuabezaleel · 2021-05-23T12:33:52Z

Hi @carolynvs , I'm truly sorry for a really late PR. Looking forward to the review! 🙂

carolynvs

Wonderful! 🙏🏼 Thank you for putting this together. There are just two things that I'd like to include in this pull request as well:

Mention the command and how to use it on our website so that people know about it. https://porter.sh/mixin-dev-guide/ right now points people to the skeletor repo. Can you add a quick sentence or two and a example command showing someone how to use porter mixins create?
This doesn't have any tests and it would be easy for us to miss if anything broke. Can you write a test that does the following?
1. Run the create command in a temp directory. You can mimic a test like this one to do it
2. Run make build using exec.Command in that directory and check that it doesn't return an error.

carolynvs · 2021-06-05T16:01:14Z

pkg/porter/mixins.go

+	}
+
+	replacementList := map[string]string{
+		"get.porter.sh/mixin/skeletor":       fmt.Sprintf("github.com/%s/%s", opts.AuthorName, opts.MixinName),


We will need one more flag to collect the user's github username. --author has their full name, e.g. "Carolyn Van Slyck", which won't work as a replacement for the package name. How about --username?

carolynvs · 2021-06-05T16:02:24Z

cmd/porter/mixins.go

+A flag of --author to declare the author of the mixin is also a required input.
+You can also specify where to put mixin directory. It will default to the current directory.`,
+		Example: ` porter mixin create MyMixin --author MyName
+		porter mixin create MyMixin --author MyName --dir path/to/mymixin


Suggested change

porter mixin create MyMixin --author MyName --dir path/to/mymixin

porter mixin create MyMixin --author "My Name" --username myuser --dir path/to/mymixin

Let's show them how to use a full name with spaces, and then include a new flag for the username (more on that below).

carolynvs · 2021-06-15T16:35:19Z

@joshuabezaleel Just checking in to see if you had any questions about the review feedback, or were having trouble implementing the suggestions. I'm happy to help get this PR finished up!

joshuabezaleel · 2021-08-21T05:22:04Z

Hi @carolynvs , I am really sorry for taking a really long time to make an update on this PR. Life got hard for a while, some personal issues and the COVID situation here in Indonesia turns to be really overwhelming and gets the best of me, making me not to have any more energy outside work especially on open source.

It's getting better and I think I am ready to get back to contributing to Porter. Thank you lots for the great review, I totally skipped about it while implementing and your points on flag --username and adding unit test is spot on. I've added the flag, test, and also description that you might want to look for its wording on the docs since English isn't my first and day-to-day language.

Looking forward to further review, and many more Porter issues to tackle 💪 Hope you all Porter folks are also doing well in these times ❤️

carolynvs

This looks great! I have some small changes to polish it up and then we can merge.

I have changed the target branch for your pull request to release/v1 as we are no longer adding new features to the main branch while we work on v1. Let me know if that causes you any trouble. I will resolve the merge conflict on that doc page once all of your changes are in so that I don't get in your way.

carolynvs · 2021-08-26T18:27:15Z

Makefile

 	# Wait for the documentation web server to finish rendering
 	@until docker logs porter-docs | grep -m 1  "Web Server is available"; do : ; done
-	@open "http://localhost:1313/docs/"
+	# @open "http://localhost:1313/docs/"


Looks like you were either having trouble with this line, or testing something and left this change in? Let's revert the changes to this file.

carolynvs · 2021-08-26T18:30:13Z

pkg/porter/mixins_test.go

+func TestPorter_CreateMixin(t *testing.T) {
+	p := NewTestPorter(t)
+
+	homeDir, err := os.UserHomeDir()


I'm not immediately seeing why you didn't use p.FileSystem.TempDir here? Is there a requirement that I'm missing?

carolynvs · 2021-08-26T18:32:41Z

pkg/porter/mixins_test.go

+	gotOutput := p.TestConfig.TestContext.GetOutput()
+	assert.Contains(t, wantOutput, gotOutput)
+
+	err = os.RemoveAll(porterTempDir)


It's a good practice to cleanup temp files using a defer statement immediately after you create it (so on line 74).

defer os.RemoveAll(porterTempDir)

That way if the test stops prematurely for any reason, for example if CreateMixin panicked, we can be certain that the directory will be cleaned up.

carolynvs · 2021-08-26T18:35:42Z

tests/smoke/.dockerignore

@@ -0,0 +1,4 @@
+# See https://docs.docker.com/engine/reference/builder/#dockerignore-file


Looks like you ran the smoke tests locally. There was a bug for a bit in porter where temporary files from running the smoke tests (like this file and .gitignore, dockerfile.tmp, etc) were written to the tests/smoke directory. Can you remove these from your PR? They shouldn't be checked in.

carolynvs · 2021-08-26T18:42:30Z

cmd/porter/mixins.go

+		Use:   "create NAME --author \"My Name\" --username mygithubusername [--dir /path/to/mixin/dir]",
+		Short: "Create a new mixin project based on the getporter/skeletor repository",
+		Long: `Create a new mixin project based on the getporter/skeletor repository.
+The first argument is the name of the mixin to create and is required.


suggestion: add a newline here so that when they print out the help it's not a giant paragraph. It will print a description of the command in the first paragraph and then details about how to use it in the second. I think that will be a bit easier for people to read.

carolynvs · 2021-08-26T18:44:12Z

cmd/porter/mixins.go

+	f.StringVar(&opts.AuthorName, "author", "", "Name of the mixin's author.")
+	f.StringVar(&opts.AuthorUsername, "username", "", "GitHub's username of the mixin's author.")


Here's a suggestion for an easier to read flag description:

Suggested change

f.StringVar(&opts.AuthorName, "author", "", "Name of the mixin's author.")

f.StringVar(&opts.AuthorUsername, "username", "", "GitHub's username of the mixin's author.")

f.StringVar(&opts.AuthorName, "author", "", "Your full name")

f.StringVar(&opts.AuthorUsername, "username", "", "Your GitHub username")

carolynvs · 2021-08-26T18:45:18Z

pkg/porter/mixins.go

+		}
+	}
+
+	fmt.Fprintf(p.Out, "Created %s mixin", opts.MixinName)


This ensures that after the command is run, the shell prompt isn't printed on the same line as your last output:

Suggested change

fmt.Fprintf(p.Out, "Created %s mixin", opts.MixinName)

fmt.Fprintf(p.Out, "Created %s mixin\n", opts.MixinName)

carolynvs · 2021-09-20T13:36:18Z

@joshuabezaleel Looks like this is ready for another review! I'll try to get to this today.

Only one of your commits has a line that says "signed-off-by ... Josh", which is why the DCO check is failing. If you are comfortable with git, you can fix that yourself with git rebase release/v1 --signoff. If you run into trouble signing your commits, leave a comment here that it's okay for me to squash your commits into a single one and use the signoff from your first commit.

Also there is a merge conflict on one of the files. If you can fix that, great, if not I'll resolve it when I merge your PR.

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

…ate with author's GitHub username flag later Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

2. Add unit test for `porter mixin create` command 3. Add content for `porter mixin create` command to mixin dev guide on docs Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

…ular directory Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

…st locally Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

…r reading Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

joshuabezaleel · 2021-09-21T14:15:50Z

Hi @carolynvs , sorry for the late update! Thank you so much for the really helpful inputs on the review and sorry for the unintentional changes that I've made on the last PR. I've tried to rebase the branch, resolve the conflict, and sign-off all of my commits, hopefully it doesn't cause any problems. 😅

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs

Looks great, thanks!

I have added a commit to your PR that removes some files that should not have been checked into the tests/smoke directory.

carolynvs · 2021-09-28T20:20:05Z

This will go out in the next alpha release of Porter! 🎉

joshuabezaleel · 2021-10-14T09:44:12Z

Ah sorry that I missed and just read about this, thank you lots for the commit of removing the files and merging it @carolynvs ! 😄

carolynvs · 2021-10-14T16:24:56Z

No worries! This PR is now included in v1.0.0-alpha.4 for anyone who wants to try it out!

joshuabezaleel requested review from carolynvs, jeremyrickard and vdice as code owners May 23, 2021 12:31

joshuabezaleel force-pushed the porter-mixin-create-command branch from b377f17 to dccb2fb Compare May 23, 2021 12:33

joshuabezaleel force-pushed the porter-mixin-create-command branch 2 times, most recently from 621ad3a to 530463b Compare May 25, 2021 11:34

carolynvs requested changes Jun 5, 2021

View reviewed changes

joshuabezaleel force-pushed the porter-mixin-create-command branch from 1fa711b to 8e8f394 Compare August 20, 2021 15:45

carolynvs changed the base branch from main to release/v1 August 26, 2021 18:38

carolynvs requested changes Aug 26, 2021

View reviewed changes

joshuabezaleel force-pushed the porter-mixin-create-command branch from c1a46c8 to 01a8bcc Compare September 19, 2021 18:08

joshuabezaleel force-pushed the porter-mixin-create-command branch from 01a8bcc to deed09e Compare September 20, 2021 13:43

joshuabezaleel added 11 commits September 20, 2021 20:57

Add porter mixin create command

f9b7769

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Remove extra new line

ccf7e23

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Use two words separated by space to denote it's a name and differenti…

4a444ba

…ate with author's GitHub username flag later Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

1. Add new flag --username

db60d9b

2. Add unit test for `porter mixin create` command 3. Add content for `porter mixin create` command to mixin dev guide on docs Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Use Porter's tempDir for the test and defer the removal of the partic…

db13847

…ular directory Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Delete .dockerignore file on /tests as the result of running smoke te…

cd6c0bf

…st locally Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Add new line for command description to separate paragraphs for easie…

33ed6b0

…r reading Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Change flag description

dd580df

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Add newline for info on succeed create new mixin

b676984

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Updated doc

b6d5088

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Fix unit test

7532aff

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

joshuabezaleel force-pushed the porter-mixin-create-command branch from deed09e to 7532aff Compare September 20, 2021 13:58

Remove files that should not be checked in

f101587

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs approved these changes Sep 28, 2021

View reviewed changes

carolynvs merged commit 9b87b62 into getporter:release/v1 Sep 28, 2021

	porter mixin create MyMixin --author MyName --dir path/to/mymixin
	porter mixin create MyMixin --author "My Name" --username myuser --dir path/to/mymixin

		@@ -0,0 +1,4 @@
		# See https://docs.docker.com/engine/reference/builder/#dockerignore-file

		f.StringVar(&opts.AuthorName, "author", "", "Name of the mixin's author.")
		f.StringVar(&opts.AuthorUsername, "username", "", "GitHub's username of the mixin's author.")

	fmt.Fprintf(p.Out, "Created %s mixin", opts.MixinName)
	fmt.Fprintf(p.Out, "Created %s mixin\n", opts.MixinName)

Conversation

joshuabezaleel commented May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Uh oh!

joshuabezaleel commented May 23, 2021

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolynvs commented Jun 15, 2021

Uh oh!

joshuabezaleel commented Aug 21, 2021

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolynvs commented Sep 20, 2021

Uh oh!

joshuabezaleel commented Sep 21, 2021

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

carolynvs commented Sep 28, 2021

Uh oh!

joshuabezaleel commented Oct 14, 2021

Uh oh!

carolynvs commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joshuabezaleel commented May 23, 2021 •

edited

Loading