adding functionality to porter create in a specified directory #2778

sbdtu5498 · 2023-06-09T02:54:43Z

What does this change

Appropriate feature has been added and locally tested as well.

What issue does it fix

If there is not an existing issue, please make sure we have context on why this change is needed. See our Contributing Guide for examples of when an existing issue isn't necessary.

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

troy0820

Some nits and a question/suggestion about the create command.

troy0820 · 2023-06-09T13:36:46Z

cmd/porter/bundle.go

 		RunE: func(cmd *cobra.Command, args []string) error {
-			return p.Create()
+			bundleName := "mybundle" // Default value


Is this necessary? This command will error if bundle name is not passed which I believe is what we want.

porter create // will error out porter create mybundle //will not fail

Suggested change

bundleName := "mybundle" // Default value

var bundleName string

@troy0820 currently it will not throw any error. It instead will create a bundle with the default name "mybundle". But if we want to throw an error if the argument isn't passed, then sure, I will fix it.

@schristoff @phillipgibson Do we want the behavior to error (error being a word used to describe not executing the command but show the usage) when the argument for the create bundle isn't given to make the user do this?

porter create [bundle]

Or do we want to create the bundle with the default mybundle in the directory if the argument isn't given?

But if we want to throw an error if the argument isn't passed, then sure, I will fix it.

@sbdtu5498
I'm not quite sure but I want to see what is the desired behavior that we want and asking the community. From the issue I believe we want to emulate the same behavior as the below clis.

helm create requires 1 argument
cargo new requires 1 argument

@troy0820 sure thing! Once we get more reviews on how it should be handled, I will add the required changes.

My bias is towards helm, and feeling that most users of our product are familiar with helm.
~~In that case, let's force the user to do the hard thing, and pick a name. Error if they don't pass in an argument, because making assumptions can cause issues down the line.~~ :)
On second thought, this is user facing.
There could be users who have scripts that rely on Porter to do stuff in the current directory.
For now, if they don't pass in an argument, we do it in the current directory. If they pass in an argument, then we create the directory.

Sure thing, this seems feasible.

troy0820 · 2023-06-09T13:40:18Z

pkg/porter/create.go

+	// Create a directory with the given bundle name
+	err := os.Mkdir(bundleName, os.ModePerm)
+	if err != nil {
+		return fmt.Errorf("failed to create directory for bundle: %w", err)


Suggested change

return fmt.Errorf("failed to create directory for bundle: %w", err)

return fmt.Errorf("failed to create directory for bundle %s: %w", bundleName, err)

pkg/porter/create.go

troy0820

Just a couple of comments and changes requested.

troy0820 · 2023-06-20T16:52:40Z

pkg/porter/create.go

+		bundleName = "."
+	}
+
+	fmt.Fprintf(p.Out, "creating porter configuration for %s\n", bundleName)


If bundle name is an empty string this will say creating porter configuration for ..

Could getting the cwd ~~os.Getwd()~~ p.FileSystem.Getwd() be a better way to do this?

Setting bundle name to ~~os.Getwd()~~ p.FileSystem.Getwd() can do two things:

It can default to the current working directory and allow this to print what it needs to be.

It will still work with what you have for the writing of the files to the filepath below.

You can get the basePath with porter.FileSystem.Getwd() and use that for the bundleName if it is blank.

sure this one is done

troy0820 · 2023-06-20T16:54:08Z

pkg/porter/create.go


-	err := p.CopyTemplate(p.Templates.GetManifest, config.Name)
+	// Create a directory with the given bundle name
+	err := os.MkdirAll(bundleName, os.ModePerm)


Suggested change

err := os.MkdirAll(bundleName, os.ModePerm)

err := os.MkdirAll(bundleName, pkg.FileModeDirectory)

troy0820 · 2023-06-20T16:55:04Z

pkg/porter/create.go

+	// Create a directory with the given bundle name
+	err := os.MkdirAll(bundleName, os.ModePerm)
+	if err != nil {
+		return fmt.Errorf("failed to create directory for bundle: %w", err)


Suggested change

return fmt.Errorf("failed to create directory for bundle: %w", err)

return fmt.Errorf("failed to create directory for bundle %s: %w", bundleName, err)

troy0820 · 2023-06-20T17:06:06Z

pkg/porter/create_test.go

+	p := NewTestPorter(t)
+	defer p.Close()
+
+	err := p.Create("mybundle")


If we pass in mybundle/yourbundle/superbundle will this work?

Helm has a thing where it will create the directory (base directory) if the others exists. So it os.Stat's the previous directories before making a helm create in the last (superbundle) "directory" assuming the last isn't a file.

We are creating the whole path regardless if it's a file at that path to include a directory of that and the bundle name will be mybundle/yourbundle/superbundle.

We may need to Stat the filepath of the directories if they are passed like this to ensure we do not have a bundle name with slashes, etc.

Helm has an example of this in their create flow.

Oh I am sorry I was a bit busy at the moment to answer. Let me get back on to this

If we pass in mybundle/yourbundle/superbundle will this work?

Yes this will work. But there's a catch. It would update the directory if it exists. Now I want to ask should we block the user from running porter create or porter create mybundle if any of the related files exist or should we stick with the behavior that it is in.

Yes this will work.

I'm curious as what the name will be. Will it be mybundle/yourbundle/superbundle

But there's a catch. It would update the directory if it exists.

I don't think that's a problem. Helm does the same thing. If there exists a directory, it will overwrite what's in there if the target exists.

Now I want to ask should we block the user from running porter create or porter create mybundle if any of the related files exist or should we stick with the behavior that it is in.

The existing functionality will allow you to create a bundle in the current directory if anything exits there and overwrite what is laid down on disks as an overwritten file. This behavior should stay and expect that to be the case with the target directory (if new created, if old overwrite)

I'm curious as what the name will be. Will it be mybundle/yourbundle/superbundle

it will be named superbundle. And will be inside youbundle folder which will be inside, superbundle. It would work exactly like how helm create works

The existing functionality will allow you to create a bundle in the current directory if anything exits there and overwrite what is laid down on disks as an overwritten file. This behavior should stay and expect that to be the case with the target directory (if new created, if old overwrite)

Sure I have made those small changes will push it in few moments. Let me know if anything else is required.

@troy0820 now it is 100% similar to helm create. I have also written the test cases to prove the point and looked at them locally as well. Kindly take a look.

schristoff · 2023-06-23T03:11:46Z

cmd/porter/bundle.go

 		Short: "Create a bundle",
-		Long:  "Create a bundle. This generates a porter bundle in the current directory.",
+		Long:  "Create a bundle. This generates a porter bundle with the directory with the specified name or in the root directory if no name is provided.",


I would leave this as "current directory" or use "working directory" instead of "root directory"
Root directory can have a lot of different meanings depending on the users background, and in my interpretation that is not where I would want my bundle. :)

pkg/porter/create.go

schristoff · 2023-06-23T03:14:52Z

pkg/porter/create.go

+	if bundleName == "" {
+		bundleName = p.FileSystem.Getwd() // Use the current directory if no directory is passed


Suggested change

if bundleName == "" {

bundleName = p.FileSystem.Getwd() // Use the current directory if no directory is passed

// Use the current directory if no directory is passed

if bundleName == "" {

bundleName = p.FileSystem.Getwd()

schristoff · 2023-06-23T03:15:52Z

pkg/porter/create.go

+		bundleName = p.FileSystem.Getwd() // Use the current directory if no directory is passed
+	}
+
+	fmt.Fprintf(p.Out, "creating porter configuration in %s\n", bundleName)


I feel like this should be moved to the end of the function, so if we succeed, this outputs

Okay, will move it.

schristoff · 2023-06-23T03:19:25Z

cmd/porter/bundle.go

 		RunE: func(cmd *cobra.Command, args []string) error {
-			return p.Create()
+			bundleName := "" // Default value


Suggested change

bundleName := "" // Default value

// By default we create the bundle in the current directory

bundleName := ""

Minor comment nit

By the way it is hard to do suggestions for comments in github because it won't let me use tabs :( so it may look a little weird

Yeah, but I got it anyways! would fix them up.

schristoff · 2023-07-05T21:45:03Z

Hey @sbdtu5498 - just following up on this to see if you need any help! :)

sbdtu5498 · 2023-07-06T19:40:28Z

Extremely sorry for the delays @schristoff. This thing really got out of my mind. Thanks a lot for reminding me as well. Everything has been fixed. Take a look.

troy0820 · 2023-07-07T13:59:33Z

pkg/porter/create.go

+	// It takes both the relative path and absolute path into consideration.
+	// For example if we want to create a bundle named mybundle in an existing directory /home/user we can call porter create /home/user/mybundle or porter create mybundle in the /home/user directory.
+	// If we are in a directory /home/user and we want to create mybundle in the directory /home/user/directory given the directory exists, 
+	// we can call porter create directory/mybundle from the /home/user directory or with any relative paths' combinations that one can come up with.


~~To clarify, when we do this porter create scripts/bundles/porter is the name going to be this as well?~~ I feel like we may have to split on the / to os.Stat the directory path if it doesn't/does exist but only use the last name from the / when creating the bundle. Do we not have to walk the directory structure?

No the name will be let say if you create /home/sbdtu5498/gentoo/nix/os. let say both gentoo and nix exist then it will work and will create bundle in the directory named os. If os doesn't exist, it will create a directory called os. But if nix doesn't exist, then it will throw error no such directory exist. This is the exact behavior that helm commands show. I tried to closely replicate it. Hope it makes sense. I will add few more descriptor comments.

// Only condition to use porter create with absolute and relative paths is that all the directories in the path except the last one should strictly exist.

I will add this line here. Now it will make sense.

troy0820 · 2023-07-07T14:01:02Z

pkg/porter/create.go

+		if err != nil {
+			return fmt.Errorf("failed to create directory for bundle: %w", err)
+		}
+	} else if err != nil {


nit: Because the err != nil in both places, you can check for the err != nil first and then checi to see if the os.IsNotExist(err)

troy0820 · 2023-07-07T14:03:44Z

pkg/porter/create_test.go

+
+func TestCreateInDirectory(t *testing.T) {
+	// Create a temporary directory for testing
+	tempDir, err := ioutil.TempDir("", "porter-test")


Suggested change

tempDir, err := ioutil.TempDir("", "porter-test")

tempDir, err := os.TempDir("", "porter-test")

ioutil.TempDir is deprecated and will not pass the gate for staticcheck/go vet

troy0820 · 2023-07-07T14:04:36Z

pkg/porter/create_test.go

+
+func TestCreateInChildDirectoryWithExistingParentDirectory(t *testing.T) {
+	// Create a temporary directory for testing
+	tempDir, err := ioutil.TempDir("", "porter-test")


Suggested change

tempDir, err := ioutil.TempDir("", "porter-test")

tempDir, err := os.TempDir("", "porter-test")

troy0820 · 2023-07-07T14:04:46Z

pkg/porter/create_test.go

+
+func TestCreateInChildDirectoryWithoutExistingParentDirectory(t *testing.T) {
+	// Create a temporary directory for testing
+	tempDir, err := ioutil.TempDir("", "porter-test")


Suggested change

tempDir, err := ioutil.TempDir("", "porter-test")

tempDir, err := os.TempDir("", "porter-test")

troy0820 · 2023-07-07T14:05:08Z

pkg/porter/create_test.go

@@ -1,25 +1,28 @@
 package porter

 import (
+	"io/ioutil"


Suggested change

"io/ioutil"

troy0820

Just some nits but passing through the gates for static check we need to change the ioutil import and use os.TempDir

This is looking good. Definitely appreciate your patience.

Also, a question about the bundlename to make sure we don't fail prematurely if we need to walk the directories before creating/not creating them.

sbdtu5498 · 2023-07-07T20:36:07Z

This is looking good. Definitely appreciate your patience.

No problem at all and sorry for some delays on my part as well.

Also, a question about the bundle name to make sure we don't fail prematurely if we need to walk the directories before creating/not creating them.

Have answered it above.

Just some nits but passing through the gates for static check we need to change the ioutil import and use os.TempDir

Fixing it ASAP.

Signed-off-by: Sanskar Bhushan <[email protected]>

sbdtu5498 · 2023-07-07T21:47:27Z

@troy0820 please take a look. I think there would be some merge conflicts. I will rebase the branch later if that's fine.

troy0820

Last change needed but this is so close @sbdtu5498. Thank you for your contribution and appreciate your patience with us.

troy0820 · 2023-07-27T14:39:23Z

pkg/porter/create.go

+			// If we are in a directory /home/user and we want to create mybundle in the directory /home/user/directory given the directory exists,
+			// we can call porter create directory/mybundle from the /home/user directory or with any relative paths' combinations that one can come up with.
+			// Only condition to use porter create with absolute and relative paths is that all the directories in the path except the last one should strictly exist.
+			err = os.Mkdir(bundleName, os.ModePerm)


Suggested change

err = os.Mkdir(bundleName, os.ModePerm)

err = os.MkdirAll(bundleName, os.ModePerm)

This will make sure that this creates bundles such as porter create mybundle/newestbundle

troy0820 · 2023-07-27T14:39:44Z

pkg/porter/create.go

+	_, err := os.Stat(bundleName)
+	if err != nil {
+		if os.IsNotExist(err) {
+			// This code here attempts to create the directory in which bundle needs to be created,


This comment is HUGE. Can we shorten this some?

schristoff · 2023-07-28T15:57:26Z

Oh @sbdtu5498 I am so sorry, I never saw that you updated this awhile back. Once those last comments of Troy's are merged I will look over and get this merged ASAP.

ludfjig · 2023-08-23T21:23:22Z

Hi @sbdtu5498 , are you still working on this?

sbdtu5498 requested review from schristoff, sgettys and bdegeeter as code owners June 9, 2023 02:54

troy0820 reviewed Jun 9, 2023

View reviewed changes

schristoff reviewed Jun 11, 2023

View reviewed changes

pkg/porter/create.go Show resolved Hide resolved

sbdtu5498 force-pushed the adding-new-feature-in-porter-create branch 2 times, most recently from ddceee7 to 2d4c508 Compare June 20, 2023 16:33

sbdtu5498 requested review from schristoff and troy0820 June 20, 2023 16:33

troy0820 requested changes Jun 20, 2023

View reviewed changes

sbdtu5498 force-pushed the adding-new-feature-in-porter-create branch from 2d4c508 to ee0ecf5 Compare June 23, 2023 00:52

sbdtu5498 requested a review from troy0820 June 23, 2023 00:53

schristoff reviewed Jun 23, 2023

View reviewed changes

sbdtu5498 force-pushed the adding-new-feature-in-porter-create branch from ee0ecf5 to 1b4918b Compare July 6, 2023 19:39

sbdtu5498 force-pushed the adding-new-feature-in-porter-create branch 2 times, most recently from 02a9591 to 6b1f2b1 Compare July 6, 2023 19:52

troy0820 reviewed Jul 7, 2023

View reviewed changes

pkg/porter/create_test.go Outdated

@@ -1,25 +1,28 @@

package porter

import (

"io/ioutil"

Copy link

Member

troy0820 Jul 7, 2023

Choose a reason for hiding this comment

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

Suggested change

"io/ioutil"

troy0820 requested changes Jul 7, 2023

View reviewed changes

adding functionality to port create in a specified directory

c4d00bd

Signed-off-by: Sanskar Bhushan <[email protected]>

sbdtu5498 force-pushed the adding-new-feature-in-porter-create branch from 6b1f2b1 to c4d00bd Compare July 7, 2023 21:40

troy0820 requested changes Jul 27, 2023

View reviewed changes

ludfjig mentioned this pull request Sep 1, 2023

add optional [bundle-name] for porter create [bundle-name] #2892

Merged

4 tasks

schristoff closed this Sep 18, 2023

	bundleName := "mybundle" // Default value
	var bundleName string

	return fmt.Errorf("failed to create directory for bundle: %w", err)
	return fmt.Errorf("failed to create directory for bundle %s: %w", bundleName, err)

	err := os.MkdirAll(bundleName, os.ModePerm)
	err := os.MkdirAll(bundleName, pkg.FileModeDirectory)

		if bundleName == "" {
		bundleName = p.FileSystem.Getwd() // Use the current directory if no directory is passed

	bundleName := "" // Default value
	// By default we create the bundle in the current directory
	bundleName := ""

	tempDir, err := ioutil.TempDir("", "porter-test")
	tempDir, err := os.TempDir("", "porter-test")

	err = os.Mkdir(bundleName, os.ModePerm)
	err = os.MkdirAll(bundleName, os.ModePerm)

adding functionality to porter create in a specified directory #2778

adding functionality to porter create in a specified directory #2778

Conversation

sbdtu5498 commented Jun 9, 2023 • edited Loading

What does this change

What issue does it fix

Checklist

Reviewer Checklist

troy0820 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schristoff Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troy0820 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbdtu5498 Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbdtu5498 Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schristoff commented Jul 5, 2023

sbdtu5498 commented Jul 6, 2023 • edited Loading

troy0820 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troy0820 left a comment

Choose a reason for hiding this comment

sbdtu5498 commented Jul 7, 2023

sbdtu5498 commented Jul 7, 2023

troy0820 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schristoff commented Jul 28, 2023

ludfjig commented Aug 23, 2023

sbdtu5498 commented Jun 9, 2023 •

edited

Loading

schristoff Jun 11, 2023 •

edited

Loading

sbdtu5498 Jun 20, 2023 •

edited

Loading

sbdtu5498 Jun 22, 2023 •

edited

Loading

sbdtu5498 commented Jul 6, 2023 •

edited

Loading

troy0820 Jul 7, 2023 •

edited

Loading