Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move unexport to new subctl implementation #1983

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Move unexport to new subctl implementation #1983

merged 2 commits into from
Apr 13, 2022

Conversation

Jaanki
Copy link
Contributor

@Jaanki Jaanki commented Mar 24, 2022

Signed-off-by: Janki Chhatbar [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1983/Jaanki/unexport
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

pkg/service/export.go Outdated Show resolved Hide resolved
cmd/subctl/root.go Outdated Show resolved Hide resolved
@Jaanki Jaanki requested a review from skitt March 31, 2022 08:54
cmd/subctl/unexport.go Outdated Show resolved Hide resolved
pkg/service/export.go Outdated Show resolved Hide resolved
Comment on lines 33 to 35
if k8serrors.IsNotFound(err) {
status.Success("Service %s not found", svcName)
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be reported as success.

Suggested change
if k8serrors.IsNotFound(err) {
status.Success("Service %s not found", svcName)
return nil
}
if k8serrors.IsNotFound(err) {
status.Failure("Service %s/%s was not previously exported", namespace, svcName)
return err
}

pkg/service/unexport.go Outdated Show resolved Hide resolved
cmd/subctl/root.go Outdated Show resolved Hide resolved
@Jaanki Jaanki requested a review from tpantelis April 11, 2022 15:09
@@ -84,3 +84,7 @@ func compareFiles(file1, file2 string) (bool, error) {

return bytes.Equal(first, second), nil
}

func addNamespaceFlag(cmd *cobra.Command, namespace string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The caller needs to pass the address of namespace. However I don't it makes sense to generalize the option name and description, ie we lose specific useful information, eg "namespace of the service to be exported" is better than "namespace to use". We could pass the name/description but I don't think it's worth it since this function just wraps one lint of code. Therefore, just inline cmd.Flags().StringVarP(...).

pkg/service/unexport.go Outdated Show resolved Hide resolved
pkg/service/unexport.go Outdated Show resolved Hide resolved
@Jaanki Jaanki requested a review from tpantelis April 12, 2022 17:10
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Apr 13, 2022
@dfarrell07
Copy link
Member

Really weird MD link check error, happed again after re-run:

fatal: unsafe repository ('/github/workspace' is owned by someone else)
To add an exception for this directory, call:

	git config --global --add safe.directory /github/workspace

@tpantelis tpantelis merged commit b71aa7b into submariner-io:devel Apr 13, 2022
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1983/Jaanki/unexport]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
None yet
5 participants