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

ludfjig · 2023-09-01T16:57:17Z

What does this change

Takes over inactive PR #2778.

porter create now accepts an optional parameter [bundle-name]. A directory bundle-name will be created if it doesn't already exist, and the bundle will be created in there. The bundle name in porter.yaml will also be set to bundle-name.

I think we should consider making this argument required instead of optional even though it's a user facing change, see #2778 (comment). It's also what helm does.

Would appreciate feedback on how to word the command help text, I wasn't able to put it as succinctly as I wanted, and also want to mention that a directory will be created.

What issue does it fix

Closes #2772

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

pkg/porter/create.go

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

Signed-off-by: Ludvig Liljenberg <[email protected]>

cmd/porter/bundle.go

pkg/porter/create.go

schristoff

Looking good so far! 👍🏻

troy0820

Couple of nits and some changes needed but this looking good.

troy0820 · 2023-09-05T19:12:02Z

pkg/porter/create.go

+	}
+
+	var err error
+	if bundleName == "" {


Your logic is repeating itself if it falls through the bundleName case being empty. Returning early with nil would make this check implicit if it isn’t filled out here.

The logic is the same in both cases it’s just where it gets executed. This can make it easier to understand and less things to write.

hopefully this is addressed now that it's 2 different functions

pkg/porter/create.go

troy0820 · 2023-09-05T19:16:14Z

pkg/porter/create.go

+		_, err := os.Stat(bundleName)
+		if err != nil && errors.Is(err, os.ErrNotExist) {
+			err = os.Mkdir(bundleName, os.ModePerm)
+			if err != nil {


Your error is not nil so you don’t have to check it again. Your logic above can be changed to check if it is nil and the. Check if it is the error you are looking for. If it is still nil, you can fall through to the not nil case and not re check the error.

This applies to line 23

something like this?

if err != nil { if errors.Is(err, os.ErrNotExist) { err = os.MkdirAll(dir, os.ModePerm) } if err != nil { return fmt.Errorf("failed to create directory for bundle: %w", err) } }

troy0820 · 2023-09-05T19:18:21Z

pkg/porter/create.go

+			if err != nil {
+				return nil, err
+			}
+			content = []byte(strings.ReplaceAll(string(content), "porter-hello", bundleName))


Are we removing the porter-hello hardcoded?

Yea, the porter-hello will be replaced

Signed-off-by: Ludvig Liljenberg <[email protected]>

pkg/porter/create.go

Signed-off-by: Ludvig Liljenberg <[email protected]>

azure-pipelines · 2023-09-05T23:28:16Z

Azure Pipelines successfully started running 1 pipeline(s).

schristoff · 2023-09-05T23:28:29Z

/azp run porter-integration

azure-pipelines · 2023-09-05T23:28:37Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Ludvig Liljenberg <[email protected]>

schristoff · 2023-09-05T23:55:45Z

pkg/porter/create.go

+	if err := p.CopyTemplate(p.Templates.GetManifest, filepath.Join(destinationDir, config.Name)); err != nil {
+		return err
+	}
+	return p.copyAllTemplatesExceptPorterYaml(destinationDir)


Suggested change

return p.copyAllTemplatesExceptPorterYaml(destinationDir)

// pass in current directory

return p.copyAllTemplatesExceptPorterYaml(".")

schristoff

One last nit and we are good to go

ludfjig commented Sep 1, 2023

View reviewed changes

pkg/porter/create.go Outdated Show resolved Hide resolved

ludfjig marked this pull request as ready for review September 1, 2023 23:21

ludfjig requested review from schristoff, sgettys, bdegeeter and troy0820 as code owners September 1, 2023 23:21

ludfjig changed the title ~~adding functionality to port create in a specified directory~~ add optional [bundle-name] for porter create **[bundle-name]** Sep 1, 2023

ludfjig changed the title ~~add optional [bundle-name] for porter create **[bundle-name]**~~ add optional [bundle-name] for porter create [bundle-name] Sep 1, 2023

sbdtu5498 and others added 6 commits September 5, 2023 10:20

adding functionality to port create in a specified directory

1c81209

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

add warning when overwriting file + fix comments

ecea4ef

Signed-off-by: Ludvig Liljenberg <[email protected]>

fixes

851b923

Signed-off-by: Ludvig Liljenberg <[email protected]>

fix

3771be2

Signed-off-by: Ludvig Liljenberg <[email protected]>

forgot this file

4f10522

Signed-off-by: Ludvig Liljenberg <[email protected]>

fix

f7f2395

Signed-off-by: Ludvig Liljenberg <[email protected]>

ludfjig force-pushed the porter_create branch from 42f2dcd to f7f2395 Compare September 5, 2023 17:20

fixed tests

60a9ce0

Signed-off-by: Ludvig Liljenberg <[email protected]>

schristoff reviewed Sep 5, 2023

View reviewed changes

cmd/porter/bundle.go Outdated Show resolved Hide resolved

schristoff reviewed Sep 5, 2023

View reviewed changes

cmd/porter/bundle.go Show resolved Hide resolved

schristoff reviewed Sep 5, 2023

View reviewed changes

pkg/porter/create.go Outdated Show resolved Hide resolved

schristoff reviewed Sep 5, 2023

View reviewed changes

troy0820 requested changes Sep 5, 2023

View reviewed changes

ludfjig force-pushed the porter_create branch 2 times, most recently from a59b9e8 to 8c35a0c Compare September 5, 2023 21:38

feedback

7fa75da

Signed-off-by: Ludvig Liljenberg <[email protected]>

ludfjig force-pushed the porter_create branch from 8c35a0c to 7fa75da Compare September 5, 2023 21:40

ludfjig added 2 commits September 5, 2023 14:46

fixed unittest

9a6666d

Signed-off-by: Ludvig Liljenberg <[email protected]>

added test for nested name

f5c75d2

Signed-off-by: Ludvig Liljenberg <[email protected]>

ludfjig force-pushed the porter_create branch from a3a2db1 to f5c75d2 Compare September 5, 2023 21:54

updated to use the porter io writer for stderr

bc4b1b0

Signed-off-by: Ludvig Liljenberg <[email protected]>

schristoff reviewed Sep 5, 2023

View reviewed changes

pkg/porter/create.go Outdated Show resolved Hide resolved

fixed perm code + test

a764dbd

Signed-off-by: Ludvig Liljenberg <[email protected]>

feedback

dbfac3b

Signed-off-by: Ludvig Liljenberg <[email protected]>

schristoff reviewed Sep 5, 2023

View reviewed changes

schristoff approved these changes Sep 5, 2023

View reviewed changes

troy0820 approved these changes Sep 6, 2023

View reviewed changes

schristoff merged commit 235a441 into getporter:main Sep 6, 2023
16 checks passed

This was referenced Sep 11, 2023

Revert removal of logging when doing porter create #2899

Closed

revert removal of logging for porter create #2900

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ludfjig commented Sep 1, 2023 •

edited

Loading

schristoff left a comment

troy0820 left a comment

troy0820 Sep 5, 2023

ludfjig Sep 5, 2023

troy0820 Sep 5, 2023

troy0820 Sep 5, 2023

ludfjig Sep 5, 2023

troy0820 Sep 5, 2023

ludfjig Sep 5, 2023

azure-pipelines bot commented Sep 5, 2023

schristoff commented Sep 5, 2023

azure-pipelines bot commented Sep 5, 2023

schristoff Sep 5, 2023

schristoff left a comment

	return p.copyAllTemplatesExceptPorterYaml(destinationDir)
	// pass in current directory
	return p.copyAllTemplatesExceptPorterYaml(".")

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

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

Conversation

ludfjig commented Sep 1, 2023 • edited Loading

What does this change

What issue does it fix

Checklist

Reviewer Checklist

schristoff left a 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

Choose a reason for hiding this comment

azure-pipelines bot commented Sep 5, 2023

schristoff commented Sep 5, 2023

azure-pipelines bot commented Sep 5, 2023

Choose a reason for hiding this comment

schristoff left a comment

Choose a reason for hiding this comment

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

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

ludfjig commented Sep 1, 2023 •

edited

Loading