-
Notifications
You must be signed in to change notification settings - Fork 707
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
fix for Flux plugin requires an additional config getter #3560 #3582
fix for Flux plugin requires an additional config getter #3560 #3582
Conversation
gfichtenholt
commented
Oct 12, 2021
- a few integration tests for flux v2 plug in were added/refactored
So there is quite a bit of "noise" in this PR, which for the most part feel free to ignore. The main things to double check is in utils.go, called |
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.
Excellent, thanks Greg.
@@ -228,7 +223,7 @@ func kubeGetPodNames(t *testing.T, namespace string) (names []string, err error) | |||
// will create a service account with cluster-admin privs and return the associated | |||
// Bearer token (base64-encoded) | |||
func kubeCreateAdminServiceAccount(t *testing.T, name, namespace string) (string, error) { | |||
t.Logf("+kubeCreateServiceAccount(%s,%s)", name, namespace) | |||
t.Logf("+kubeCreateAdminServiceAccount(%s,%s)", name, namespace) |
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.
phew I got scared for a second before I realised this code is part of the integration test utilities :P
// it appears that if service account is deleted before the helmrelease object that uses it, | ||
// when you try to delete the helmrelease, the "delete" operation gets stuck and the only | ||
// way to get it "unstuck" is to edit the CRD and remove the finalizer. | ||
// So we'll cleanup the service account only after the corresponding helmrelease has been deleted |
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.
Sounds painful.
@@ -71,7 +70,7 @@ func NewServer(configGetter server.KubernetesConfigGetter) (*Server, error) { | |||
return nil, err | |||
} else { | |||
return &Server{ | |||
clientGetter: clientGetter, | |||
clientGetter: newClientGetter(configGetter), |
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, so the cache is configured with the background client while the server (handling user requests) still always uses the provided config getter (that gets authz from the context).
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.
correct
md, ok := metadata.FromIncomingContext(ctx) | ||
if !ok { | ||
return "", nil | ||
return "", fmt.Errorf("missing authorization metadata") |
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.
Excellent, thanks.