`kctrl`: Flag to create namespace when adding new repo #1113

ThomasVitale · 2023-02-26T15:42:36Z

What this PR does / why we need it:

When adding a new package repository to a cluster, it's now possible to create the installation namespace automatically by specifying the "--create-namespace" flag.

Which issue(s) this PR fixes:

Fixes gh-1001

Does this PR introduce a user-facing change?

Added a new flag `create-namespace` for creating the installation namespace when adding a new package repository to a cluster.

Additional Notes for your reviewer:

After a first review of this pull request to validate the behaviour of the new flag, I will submit a PR to update the documentation on the project website.

Review Checklist:

Follows the developer guidelines
Relevant tests are added or updated
[] Relevant docs in this repo added or updated
[] Relevant carvel.dev docs added or updated in a separate PR and there's
a link to that PR
Code is at least as readable and maintainable as it was before this
change

Additional documentation e.g., Proposal, usage docs, etc.:

praveenrewar · 2023-02-28T17:34:31Z

cli/pkg/kctrl/cmd/package/repository/add_or_update.go

+
+		_, err = coreClient.CoreV1().Namespaces().Create(context.Background(), namespace, metav1.CreateOptions{})
+		if err != nil {
+			o.ui.PrintLinef("The namespace %s already exists", o.NamespaceFlags.Name)


We should probably return the error if it's not of type IsAlreadyExists and ignore if it already exists.

Good point! I changed it as suggested.

100mik

Some thoughts!

In addition to these looks like there is a redundant log line being printed on line 187
(More specifically: o.ui.PrintLinef("Waiting for package repository to be added"))
That can be removed as a part of this PR but does not block the PR itself!

cli/pkg/kctrl/cmd/package/repository/add_or_update.go

praveenrewar · 2023-03-03T08:09:02Z

cli/pkg/kctrl/cmd/package/repository/add_or_update.go

+			if errors.IsAlreadyExists(err) {
+				o.statusUI.PrintMessagef("The namespace '%s' already exists", o.NamespaceFlags.Name)
+			} else {
+				return err
+			}


Suggested change

if errors.IsAlreadyExists(err) {

o.statusUI.PrintMessagef("The namespace '%s' already exists", o.NamespaceFlags.Name)

} else {

return err

}

if !errors.IsAlreadyExists(err) {

return err

}

o.statusUI.PrintMessagef("The namespace '%s' already exists", o.NamespaceFlags.Name)

I have refactored that part to print only a message if the namespace is created as suggested in the previous comment.

praveenrewar · 2023-03-09T15:22:18Z

cli/pkg/kctrl/cmd/package/repository/add_or_update.go

@@ -141,6 +145,30 @@ func (o *AddOrUpdateOptions) Run(args []string) error {
 		return err
 	}

+	if o.CreateNamespace {
+		o.statusUI.PrintMessagef("Creating namespace '%s'", o.NamespaceFlags.Name)


Should we remove this line as well? Or you already did and forgot to push the change?

I think I am good with the changes, once this is resolved ^

Thanks for catching this. You'd think after several commits I'd learn the lesson about signing off commits, but I keep missing that and had to rebase (my Git is configured with the -S parameter for signing, but the DCA sign-off is with -s). I guess while doing that, this message popped up again.

cli/test/e2e/package_repository_test.go

When adding a new package repository to a cluster, it's now possible to create the installation namespace automatically by specifying the "--create-namespace" flag. Fixes gh-1001 Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

100mik

Changes look good to me! Thank you so much for contributing

@praveenrewar feel free to merge this away if it looks good to you 🚀

praveenrewar · 2023-04-05T10:18:42Z

cli/test/e2e/package_repository_test.go

+	logger.Section("creating a repository in a namespace that already exists", func() {
+		kubectl.Run([]string{"create", "namespace", existingRepoNamespace})


We could probably just use env.Namespace instead of this. It would remove the overhead of creating/deleting one namespace.

Sorry didn't take a look at the tests earlier 😅

I considered doing that, but then opted for creating it explicitly to make the test case resemble the current 2-steps way of adding a repo as opposed to the changes I made in the PR (1. create the namespace, 2. add a repo). But I can change it to use the general one, if you want :)

Updated the tests here to re-use env.Namespace: 893f9e7

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

praveenrewar

LGTM! Thank you so much for the contribution 🚀

ThomasVitale · 2023-04-05T16:12:43Z

@100mik @praveenrewar thanks a lot for your guidance and review

ThomasVitale changed the title ~~Option to create namespace when adding new repo~~ kctrl: Flag to create namespace when adding new repo Feb 26, 2023

ThomasVitale force-pushed the gh-1001 branch 4 times, most recently from 81666e6 to e154ab6 Compare February 26, 2023 16:10

praveenrewar reviewed Feb 28, 2023

View reviewed changes

100mik reviewed Feb 28, 2023

View reviewed changes

cli/pkg/kctrl/cmd/package/repository/add_or_update.go Outdated Show resolved Hide resolved

cli/pkg/kctrl/cmd/package/repository/add_or_update.go Show resolved Hide resolved

ThomasVitale force-pushed the gh-1001 branch from c0a4952 to d29c99c Compare March 1, 2023 21:22

praveenrewar reviewed Mar 3, 2023

View reviewed changes

ThomasVitale force-pushed the gh-1001 branch 2 times, most recently from 1d79ada to 0be3e96 Compare March 7, 2023 18:45

praveenrewar reviewed Mar 9, 2023

View reviewed changes

100mik reviewed Mar 24, 2023

View reviewed changes

cli/test/e2e/package_repository_test.go Show resolved Hide resolved

ThomasVitale added 4 commits March 30, 2023 17:57

Improve error handling

4bf25d2

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

Optimize status messages for namespace creation

abc777a

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

Add cleanup after tests

057bbdb

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

ThomasVitale force-pushed the gh-1001 branch from b499ac2 to 057bbdb Compare March 30, 2023 15:58

ThomasVitale added 2 commits March 30, 2023 17:58

Update test cleanup

82dac38

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

Use cleanup function for new tests

94e6bdc

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

100mik approved these changes Apr 5, 2023

View reviewed changes

praveenrewar reviewed Apr 5, 2023

View reviewed changes

Re-use existing namespace in test

893f9e7

Signed-off-by: Thomas Vitale <ThomasVitale@users.noreply.github.com>

praveenrewar approved these changes Apr 5, 2023

View reviewed changes

praveenrewar merged commit 15c92de into carvel-dev:develop Apr 5, 2023
6 checks passed

github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Apr 5, 2023

praveenrewar removed the carvel-triage This issue has not yet been reviewed for validity label Apr 5, 2023

ThomasVitale mentioned this pull request Apr 14, 2024

REQUEST: New membership for ThomasVitale carvel-dev/carvel#752

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`kctrl`: Flag to create namespace when adding new repo #1113

`kctrl`: Flag to create namespace when adding new repo #1113

ThomasVitale commented Feb 26, 2023

praveenrewar Feb 28, 2023

ThomasVitale Mar 1, 2023

100mik left a comment

praveenrewar Mar 3, 2023

ThomasVitale Mar 7, 2023

praveenrewar Mar 9, 2023

100mik Mar 24, 2023

ThomasVitale Mar 30, 2023

100mik left a comment

praveenrewar Apr 5, 2023

praveenrewar Apr 5, 2023

ThomasVitale Apr 5, 2023 •

edited

ThomasVitale Apr 5, 2023

praveenrewar left a comment

ThomasVitale commented Apr 5, 2023

		logger.Section("creating a repository in a namespace that already exists", func() {
		kubectl.Run([]string{"create", "namespace", existingRepoNamespace})

kctrl: Flag to create namespace when adding new repo #1113

kctrl: Flag to create namespace when adding new repo #1113

Conversation

ThomasVitale commented Feb 26, 2023

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional Notes for your reviewer:

Review Checklist:

Additional documentation e.g., Proposal, usage docs, etc.:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100mik 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

100mik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasVitale Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenrewar left a comment

Choose a reason for hiding this comment

ThomasVitale commented Apr 5, 2023

`kctrl`: Flag to create namespace when adding new repo #1113

`kctrl`: Flag to create namespace when adding new repo #1113

ThomasVitale Apr 5, 2023 •

edited