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

add namespace in kubernetes commands #420

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rjpmestre
Copy link
Contributor

What does this PR change?

Ensures kubectl commands include the namespace

Test coverage

  • No tests: manually tested commands while asserting namespace was included

  • DONE

Links

Issue(s): #375

  • DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Before you merge

Check How to branch and merge properly!

@cbosdo cbosdo changed the title add namespace is kubernetes commands add namespace in kubernetes commands Aug 8, 2024
mgradm/cmd/install/kubernetes/utils.go Outdated Show resolved Hide resolved
mgradm/cmd/install/kubernetes/utils.go Outdated Show resolved Hide resolved
mgradm/cmd/support/config/extractor.go Outdated Show resolved Hide resolved
shared/connection.go Outdated Show resolved Hide resolved
@@ -39,7 +45,12 @@ func extract(globalFlags *types.GlobalFlags, flags *configFlags, cmd *cobra.Comm
}

if utils.IsInstalled("kubectl") && utils.IsInstalled("helm") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the unique case where we check the backend in this function. We probably need to split that a bit better in a future PR

mgradm/cmd/uninstall/kubernetes.go Show resolved Hide resolved
mgradm/cmd/uninstall/kubernetes.go Show resolved Hide resolved
Comment on lines 88 to 95
errNamespaceCheck := utils.RunCmd("kubectl", "get", "namespace", issuerData.Namespace)
if errNamespaceCheck != nil {
errNamespaceCreate := utils.RunCmd("kubectl", "create", "namespace", issuerData.Namespace)
if errNamespaceCreate != nil {
log.Fatal().Err(err).Msg(L("Failed to create namespace"))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative (and possibly cleaner) solution is to add the namespace resource to the issuer yaml we generate above. Adding this at the beginning of the template would let kubernetes do the checks and ensure the namespace is created:

apiVersion: v1
kind: Namespace
metadata:
  labels:
  name: {{ .Namespace }}
---

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed your suggestion

mgradm/shared/kubernetes/k3s.go Outdated Show resolved Hide resolved
@rjpmestre rjpmestre force-pushed the check-namespace-definition-in-kubernetes-commands branch from a4f8ff7 to 1754f7b Compare September 4, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants