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

[internal] Switch to go hooks and images build refactoring #43

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

Conversation

duckhawk
Copy link
Member

@duckhawk duckhawk commented Mar 14, 2025

Description

We need to switch to go module hooks, and get correct package names in internal packages

What is the expected result?

Correct module structure and hooks

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

@duckhawk duckhawk added the enhancement New feature or request label Mar 14, 2025
@duckhawk duckhawk requested a review from AleksZimin March 14, 2025 06:25
@duckhawk duckhawk self-assigned this Mar 14, 2025
@duckhawk duckhawk force-pushed the change-python-hooks-to-go-and-modules-refactoring branch from fdd52c4 to 6d3b4ef Compare March 14, 2025 07:22
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
Signed-off-by: v.oleynikov <[email protected]>
@duckhawk duckhawk requested a review from astef March 14, 2025 10:24
@duckhawk duckhawk marked this pull request as ready for review March 14, 2025 10:24
fmt.Sprintf("%s.%s", consts.WEBHOOKS_CERT_CN, consts.MODULE_NAMESPACE),
fmt.Sprintf("%s.%s.svc", consts.WEBHOOKS_CERT_CN, consts.MODULE_NAMESPACE),
// %CLUSTER_DOMAIN%:// is a special value to generate SAN like 'svc_name.svc_namespace.svc.cluster.local'
fmt.Sprintf("%%CLUSTER_DOMAIN%%://%s.%s.svc", consts.WEBHOOKS_CERT_CN, consts.MODULE_NAMESPACE),
Copy link
Member

Choose a reason for hiding this comment

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

From what I see here, it is literally %CLUSTER_DOMAIN%:// (single percent sign)

"sigs.k8s.io/controller-runtime/pkg/client"
)

func NewKubeClient() (client.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate of hooks/go/050-migrate-auth-to-connection/migrate-auth-to-connection.go, let's move to common place


cl, err := NewKubeClient()
if err != nil {
fmt.Printf("%s", err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Since we're using fmt.Printf for logging, each log line should end with a newline. Error text normally don't end on a newline. Please fix it in every place. Better approach would be to use logger from pkg.HookInput

Also, there's no need to call .Error(), formatter will call it byitself, if you just pass err

err = cl.Patch(ctx, &secret, patch)
if err != nil {
fmt.Printf("[remove-sc-and-secrets-on-module-delete]: Failed to patch secret %s: %v", secret.Name, err)
continue
Copy link
Member

Choose a reason for hiding this comment

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

continue looses errors. Instead, we should collect them, e.g.: resultErr := errors.Join(resultErr, err), and check resultErr in the end. The same for ConfigMap loop, and for StorageClassList loop below.

}

fmt.Printf("[remove-sc-and-secrets-on-module-delete]: Removing %s storage class\n", sc.Name)
err = cl.Delete(ctx, &storagev1.StorageClass{
Copy link
Member

Choose a reason for hiding this comment

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

Why no just cl.Delete(ctx, &sc)? I'm not sure if deletion with just Name will work at all.

tls.key: {{ .Values.csiCeph.internal.customWebhookCert.key }}
ca.crt: {{ .Values.csiCeph.internal.customWebhookCert.ca | b64enc | quote }}
tls.crt: {{ .Values.csiCeph.internal.customWebhookCert.crt | b64enc | quote }}
tls.key: {{ .Values.csiCeph.internal.customWebhookCert.key | b64enc | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

Why? Isn't it going to break the code, which uses that cert from secret? Or the code is already broken and you're fixing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants