-
Notifications
You must be signed in to change notification settings - Fork 705
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
Store a copy of the repo secret in Kubeapps' namespace #1509
Conversation
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, just some minor comments/questions
pkg/apprepo/apprepos_handler.go
Outdated
@@ -65,7 +70,7 @@ type appRepositoriesHandler struct { | |||
kubeappsNamespace string | |||
|
|||
// The Kubernetes client using the pod serviceaccount | |||
svcKubeClient *kubernetes.Clientset | |||
svcKubeClient coreClientsetInterface |
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.
rather than defining our own interface, isn't it better to use kubernetes.Interface
instead?
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.
Yep, you're right. I'm used to defining smaller interfaces with just the functionality we need to be able to test, but in this case the test client already exists, so no need.
@@ -310,6 +341,12 @@ func secretNameForRepo(repoName string) string { | |||
return fmt.Sprintf("apprepo-%s-secrets", repoName) | |||
} | |||
|
|||
// kubeappsSecretNameForRepo returns a name suitable for recording a copy of | |||
// a per-namespace repository secret in the kubeapps namespace. | |||
func kubeappsSecretNameForRepo(repoName, namespace string) string { |
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.
isn't it better to reuse the method secretNameForRepo
? To avoid confusion between similar methods.
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.
It is re-using secretNameForRepo
in its implementation. I could have done this inline, which is perhaps what you mean? The only reason I've pulled it out to a function is because I'll need to generate the same secret name when deleting in a following PR, so rather than have the same inline code to create the kubeapps secret name, I've put it in a fn. Let me know if I missed something?
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.
It's just a bit confusing to know (if I were developing here) which method to use. For me, it'd be better to extend secretNameForRepo(name, namespace)
allowing to set the namespace empty and adding a comment saying that the namespace is only needed if the secret is meant for the kubeapps
namespace.
} else { | ||
t.Errorf("Unable to convert err to StatusError: %+v", err) | ||
} | ||
} |
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.
this test body is becoming huge, I wonder if it would be better to start splitting the different checks in different tests
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.
Yeah, I thought the same thing. I'll look at this in the following PRs.
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.
Great, thanks Andres.
pkg/apprepo/apprepos_handler.go
Outdated
@@ -65,7 +70,7 @@ type appRepositoriesHandler struct { | |||
kubeappsNamespace string | |||
|
|||
// The Kubernetes client using the pod serviceaccount | |||
svcKubeClient *kubernetes.Clientset | |||
svcKubeClient coreClientsetInterface |
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.
Yep, you're right. I'm used to defining smaller interfaces with just the functionality we need to be able to test, but in this case the test client already exists, so no need.
@@ -310,6 +341,12 @@ func secretNameForRepo(repoName string) string { | |||
return fmt.Sprintf("apprepo-%s-secrets", repoName) | |||
} | |||
|
|||
// kubeappsSecretNameForRepo returns a name suitable for recording a copy of | |||
// a per-namespace repository secret in the kubeapps namespace. | |||
func kubeappsSecretNameForRepo(repoName, namespace string) string { |
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.
It is re-using secretNameForRepo
in its implementation. I could have done this inline, which is perhaps what you mean? The only reason I've pulled it out to a function is because I'll need to generate the same secret name when deleting in a following PR, so rather than have the same inline code to create the kubeapps secret name, I've put it in a fn. Let me know if I missed something?
} else { | ||
t.Errorf("Unable to convert err to StatusError: %+v", err) | ||
} | ||
} |
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.
Yeah, I thought the same thing. I'll look at this in the following PRs.
Ref: #1496
Updates the apprepo handler so that a copy of an app repository credential is recorded in Kubeapps' own namespace.