-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
scorecard - lookup namespace from kubeconfig #3478
scorecard - lookup namespace from kubeconfig #3478
Conversation
"testing" | ||
) | ||
|
||
func TestGetKubeNamespace(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we should be writing all new tests with ginkgo/gomega. But let's get this merged and add a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
||
ns, _, err := kubeConfig.Namespace() | ||
if err != nil { | ||
return "default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "default"
namespace might not exist, and we probably shouldn't mask this error so I would return it to the CLI. On the flip side, then this would be a breaking change, although I don't think that matters too much because this command was still in alpha in the most recent release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address in a follow-up
|
||
if kubeconfigPath != "" { | ||
rules.ExplicitPath = kubeconfigPath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
if kubeconfigPath != "" { | |
rules.ExplicitPath = kubeconfigPath | |
} | |
rules.ExplicitPath = kubeconfigPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Description of the change:
add logic in scorecard command to lookup the namespace, if not set on the command line, from the
kubeconfig
Motivation for the change:
Closes #3422
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs