chore(lint): enable concurrent execution for the linter and enable formatters #5752
Conversation
github-actions
bot
added
area/lint
kind/chore
WalkthroughCI workflows and Makefile updated to remove the standalone lint job, run linting via check-diff and parallelize linting; widespread formatting, import aliasing/reordering, and signature reflow across many Go files; a few targeted functional edits in Oracle, Conjur, PushSecret, and Vault-related code. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
providers/v1/cloudru/secretmanager/resolver.go (1)
56-59: Inconsistent error context in error message.Line 58 is missing error context—it should include the
errvariable wrapped with%wto match the error handling pattern used in lines 53 and 63. This provides better debugging information when the secret resolution fails.🔎 Proposed fix
secret, err := resolvers.SecretKeyRef(ctx, kcr.kube, kcr.storeKind, kcr.namespace, &kcr.ref.AccessKeySecret) if err != nil { - return nil, fmt.Errorf("failed to resolve accessKeySecret") + return nil, fmt.Errorf("failed to resolve accessKeySecret: %w", err) }providers/v1/senhasegura/auth/iso.go (2)
72-78: Potential nil pointer dereference inAuthenticatefunction.On Line 73,
isoSession.IsoSessionFromSecretRef(...)is called on a receiver that is likelynil(sinceisoSessionwas just declared without initialization). This will cause a panic at runtime. The function should instantiate a receiver or refactor the logic to work with a package-level function or static method.🔎 Proposed fix
func Authenticate(ctx context.Context, store esv1.GenericStore, provider *esv1.SenhaseguraProvider, kube client.Client, namespace string) (isoSession *SenhaseguraIsoSession, err error) { - isoSession, err = isoSession.IsoSessionFromSecretRef(ctx, provider, store, kube, namespace) + isoSession = &SenhaseguraIsoSession{} + isoSession, err = isoSession.IsoSessionFromSecretRef(ctx, provider, store, kube, namespace) if err != nil { return nil, err } return isoSession, nil }Alternatively, consider making
IsoSessionFromSecretRefa package-level function or static method if it doesn't need receiver state.
123-124: Unhandled error fromurl.ParseRequestURI.The error returned by
url.ParseRequestURIis ignored on Line 123. If parsing fails,uwill benil, and accessingu.Pathon Line 124 will cause a panic. Handle or validate the error appropriately.🔎 Proposed fix
u, _ := url.ParseRequestURI(systemURL) - u.Path = "/iso/oauth2/token" + u, err := url.ParseRequestURI(systemURL) + if err != nil { + return "", fmt.Errorf("failed to parse system URL: %w", err) + } + u.Path = "/iso/oauth2/token"providers/v1/oracle/oracle.go (1)
596-601: Environment variable names are swapped in error messages.The error messages reference the wrong environment variables:
- Line 597: Unsetting
ResourcePrincipalVersionEnvVarbut error message mentionsResourcePrincipalRegionEnvVar- Line 600: Unsetting
ResourcePrincipalRegionEnvVarbut error message mentionsResourcePrincipalVersionEnvVarAdditionally, line 600 has a typo: "unabled" → "unable".
Proposed fix
defer func() { if uerr := os.Unsetenv(auth.ResourcePrincipalVersionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalRegionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf("unable to unset OCI SDK environment variable %s: %w", auth.ResourcePrincipalVersionEnvVar, uerr)) } if uerr := os.Unsetenv(auth.ResourcePrincipalRegionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf("unabled to unset OCI SDK environment variable %s: %w", auth.ResourcePrincipalVersionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf("unable to unset OCI SDK environment variable %s: %w", auth.ResourcePrincipalRegionEnvVar, uerr)) } vms.workloadIdentityMutex.Unlock() }()
🧹 Nitpick comments (1)
providers/v1/volcengine/provider.go (1)
49-50: Consider renaming the variable to avoid shadowing the package name.The variable
kmsshadows the importedkmspackage, which can reduce readability. Consider using a more descriptive name likekmsClientorkmsService.🔎 Suggested refactor
- kms := kms.New(sess) - return NewClient(kms), nil + kmsClient := kms.New(sess) + return NewClient(kmsClient), nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (99)
.github/workflows/ci.yml(1 hunks).golangci.yaml(1 hunks)Makefile(1 hunks)apis/externalsecrets/v1/fakes/pushremoteref.go(1 hunks)apis/externalsecrets/v1/secretstore_barbican_types.go(1 hunks)apis/externalsecrets/v1/secretstore_validator_test.go(1 hunks)apis/externalsecrets/v1beta1/secretstore_validator_test.go(1 hunks)apis/generators/v1alpha1/types_mfa.go(1 hunks)cmd/controller/certcontroller.go(1 hunks)cmd/controller/root.go(2 hunks)generators/v1/acr/acr.go(2 hunks)generators/v1/cloudsmith/cloudsmith.go(0 hunks)generators/v1/ecr/resolver.go(1 hunks)generators/v1/fake/fake.go(0 hunks)generators/v1/gcr/gcr.go(1 hunks)generators/v1/github/github.go(0 hunks)generators/v1/grafana/grafana.go(0 hunks)generators/v1/mfa/mfa.go(0 hunks)generators/v1/quay/quay.go(0 hunks)generators/v1/sts/sts.go(0 hunks)generators/v1/uuid/uuid.go(0 hunks)generators/v1/vault/vault.go(2 hunks)generators/v1/vault/vault_test.go(1 hunks)main.go(1 hunks)pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go(1 hunks)pkg/controllers/crds/crds_controller.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_secret.go(3 hunks)pkg/controllers/externalsecret/externalsecret_controller_test.go(5 hunks)pkg/controllers/pushsecret/pushsecret_controller.go(3 hunks)pkg/controllers/secretstore/common_test.go(1 hunks)pkg/controllers/webhookconfig/webhookconfig.go(1 hunks)pkg/register/generators.go(0 hunks)providers/v1/akeyless/akeyless_test.go(1 hunks)providers/v1/alibaba/kms_test.go(1 hunks)providers/v1/aws/auth/auth.go(3 hunks)providers/v1/aws/auth/auth_test.go(1 hunks)providers/v1/aws/parameterstore/parameterstore.go(1 hunks)providers/v1/aws/parameterstore/parameterstore_test.go(2 hunks)providers/v1/aws/secretsmanager/secretsmanager.go(1 hunks)providers/v1/aws/secretsmanager/secretsmanager_test.go(3 hunks)providers/v1/aws/util/errors_test.go(1 hunks)providers/v1/aws/util/provider.go(1 hunks)providers/v1/azure/keyvault/fake/fake.go(1 hunks)providers/v1/azure/keyvault/keyvault_dual_sdk_test.go(1 hunks)providers/v1/azure/keyvault/keyvault_new_sdk.go(2 hunks)providers/v1/azure/keyvault/keyvault_test.go(9 hunks)providers/v1/barbican/client.go(2 hunks)providers/v1/barbican/client_test.go(1 hunks)providers/v1/barbican/fake/mock.go(1 hunks)providers/v1/barbican/provider.go(0 hunks)providers/v1/beyondtrust/provider.go(1 hunks)providers/v1/beyondtrust/provider_test.go(2 hunks)providers/v1/chef/chef.go(1 hunks)providers/v1/chef/chef_test.go(1 hunks)providers/v1/cloudru/secretmanager/client.go(1 hunks)providers/v1/cloudru/secretmanager/resolver.go(1 hunks)providers/v1/conjur/client.go(1 hunks)providers/v1/conjur/provider.go(1 hunks)providers/v1/conjur/validate.go(1 hunks)providers/v1/doppler/client.go(1 hunks)providers/v1/doppler/provider.go(1 hunks)providers/v1/gcp/secretmanager/workload_identity_federation.go(1 hunks)providers/v1/gcp/secretmanager/workload_identity_federation_test.go(1 hunks)providers/v1/gitlab/gitlab_test.go(1 hunks)providers/v1/ibm/fake/fake.go(1 hunks)providers/v1/infisical/client.go(2 hunks)providers/v1/infisical/provider.go(11 hunks)providers/v1/keepersecurity/client_test.go(3 hunks)providers/v1/kubernetes/client_test.go(2 hunks)providers/v1/kubernetes/metadata.go(1 hunks)providers/v1/ngrok/client_test.go(1 hunks)providers/v1/ngrok/provider.go(1 hunks)providers/v1/ngrok/provider_test.go(1 hunks)providers/v1/onboardbase/client.go(1 hunks)providers/v1/onboardbase/provider.go(1 hunks)providers/v1/onepassword/onepassword_test.go(1 hunks)providers/v1/oracle/oracle.go(3 hunks)providers/v1/previder/provider.go(1 hunks)providers/v1/senhasegura/auth/iso.go(1 hunks)providers/v1/vault/auth.go(1 hunks)providers/v1/vault/auth_gcp_test.go(1 hunks)providers/v1/vault/auth_iam.go(2 hunks)providers/v1/vault/client.go(1 hunks)providers/v1/vault/client_get_all_secrets_test.go(1 hunks)providers/v1/vault/client_get_test.go(1 hunks)providers/v1/vault/client_push_test.go(1 hunks)providers/v1/vault/fake/vault.go(1 hunks)providers/v1/vault/iamauth/iamauth.go(1 hunks)providers/v1/vault/provider.go(3 hunks)providers/v1/vault/provider_test.go(1 hunks)providers/v1/volcengine/provider.go(1 hunks)providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go(3 hunks)providers/v1/yandex/certificatemanager/client/fakeclient.go(1 hunks)providers/v1/yandex/certificatemanager/client/grpcclient.go(1 hunks)providers/v1/yandex/common/provider.go(1 hunks)providers/v1/yandex/lockbox/client/fakeclient.go(1 hunks)providers/v1/yandex/lockbox/client/grpcclient.go(1 hunks)providers/v1/yandex/lockbox/lockboxsecretgetter.go(2 hunks)
💤 Files with no reviewable changes (10)
- generators/v1/grafana/grafana.go
- generators/v1/fake/fake.go
- generators/v1/sts/sts.go
- generators/v1/quay/quay.go
- generators/v1/cloudsmith/cloudsmith.go
- generators/v1/github/github.go
- pkg/register/generators.go
- generators/v1/mfa/mfa.go
- generators/v1/uuid/uuid.go
- providers/v1/barbican/provider.go
🧰 Additional context used
🧬 Code graph analysis (12)
providers/v1/gitlab/gitlab_test.go (1)
providers/v1/gitlab/fake/fake.go (1)
APIResponse(26-30)
providers/v1/vault/iamauth/iamauth.go (1)
providers/v1/vault/util/vault.go (2)
Client(51-61)JwtProviderFactory(28-28)
providers/v1/vault/auth_iam.go (2)
providers/v1/aws/auth/auth.go (1)
STSProvider(359-359)providers/v1/vault/iamauth/iamauth.go (1)
STSProvider(283-283)
pkg/controllers/externalsecret/externalsecret_controller_secret.go (2)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
Reconciler(137-153)apis/externalsecrets/v1beta1/externalsecret_types.go (2)
ExternalSecretDataFromRemoteRef(312-334)ExternalSecret(571-577)
pkg/controllers/externalsecret/externalsecret_controller_test.go (2)
apis/externalsecrets/v1/externalsecret_types.go (3)
AnnotationDataHash(686-686)LabelManaged(691-691)ExternalSecretReady(590-590)apis/externalsecrets/v1beta1/externalsecret_types.go (3)
AnnotationDataHash(581-581)LabelManaged(584-584)ExternalSecretReady(498-498)
providers/v1/yandex/lockbox/lockboxsecretgetter.go (1)
providers/v1/yandex/common/provider.go (1)
ResourceKeyType(124-124)
providers/v1/barbican/fake/mock.go (1)
providers/v1/keepersecurity/client.go (1)
Secret(105-111)
generators/v1/acr/acr.go (1)
apis/meta/v1/types.go (1)
ServiceAccountSelector(46-66)
providers/v1/oracle/oracle.go (1)
apis/meta/v1/types.go (1)
ServiceAccountSelector(46-66)
pkg/controllers/pushsecret/pushsecret_controller.go (1)
apis/externalsecrets/v1alpha1/pushsecret_types.go (3)
PushSecretStoreRef(36-53)PushSecret(261-267)SyncedPushSecretsMap(233-233)
providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go (1)
providers/v1/yandex/common/provider.go (1)
ResourceKeyType(124-124)
providers/v1/conjur/provider.go (1)
providers/v1/conjur/client.go (1)
Client(45-53)
🪛 checkmake (0.2.2)
Makefile
[warning] 149-149: Target body for "lint" exceeds allowed length of 5 (34).
(maxbodylength)
🪛 Gitleaks (8.30.0)
providers/v1/azure/keyvault/keyvault_test.go
[high] 402-402: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 615-615: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 630-630: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 789-789: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: unit-tests
- GitHub Check: check-diff
- GitHub Check: Analyze project (actions, none)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (104)
providers/v1/previder/provider.go (1)
20-32: Import organization is correctly formatted.The added blank line (line 24) properly separates standard library imports from third-party imports, following Go's import organization best practice. This is a clean formatting improvement with no functional impact.
apis/generators/v1alpha1/types_mfa.go (1)
19-23: Import formatting aligns with linting standards.The blank line added between the
metav1andsmmetaimports groups external Kubernetes API imports separately from external-secrets internal imports. This is a clean formatting improvement that aligns with the PR's goal of consistent import organization.providers/v1/azure/keyvault/keyvault_new_sdk.go (2)
342-344: LGTM!The reformatting of this error return improves readability without altering functionality or error handling behavior.
371-373: LGTM!The reformatting of this error return improves readability without altering functionality or error handling behavior.
providers/v1/akeyless/akeyless_test.go (1)
315-320: Formatting change aligns with PR objectives.The reformatting of the
makeValidAkeylessTestCase("fail unmarshal")call to span multiple lines is consistent with the PR's goal to enable formatters. The builder chain method calls remain functionally identical, and the test logic is unchanged.providers/v1/senhasegura/auth/iso.go (1)
83-89: Formatting change looks good.The multi-line function signature aligns with the PR's formatting objectives and improves readability without functional changes.
providers/v1/ibm/fake/fake.go (1)
47-50: LGTM! Clean formatting improvement.The multi-line function signature enhances readability and aligns with Go formatting best practices for functions with multiple parameters. This change is consistent with the PR's objective of enabling formatters.
pkg/controllers/secretstore/common_test.go (1)
28-33: Import organization follows Go conventions.The blank line added at line 30 properly separates internal API imports from test framework imports, improving readability and following standard Go import grouping practices (stdlib → external → test-specific).
providers/v1/ngrok/provider.go (1)
25-27: LGTM! Import reordering aligns with formatting standards.The import block reordering is consistent with the PR's objective of applying linting and formatting improvements across the codebase. No functional changes detected.
providers/v1/infisical/client.go (2)
32-32: LGTM!The import reordering improves consistency and aligns with the formatting rules enabled in this PR.
200-207: LGTM!The multi-line error formatting improves readability while maintaining identical semantics. All format parameters are correctly aligned with the format string.
providers/v1/kubernetes/client_test.go (2)
592-607: LGTM!The whitespace alignment changes in the struct literals are purely cosmetic and improve visual consistency. No functional impact.
1184-1186: LGTM!Wrapping the long JSON string across multiple lines improves readability without changing the test data. The parentheses with a trailing newline-split string is idiomatic Go for breaking long literals.
providers/v1/ngrok/provider_test.go (1)
34-36: LGTM! Import organization improved.The Ginkgo/Gomega dot-imports are now grouped separately, improving consistency with the broader formatting improvements in this PR.
providers/v1/ngrok/client_test.go (1)
31-33: LGTM! Import organization improved.The Ginkgo/Gomega dot-imports are now grouped separately, consistent with the formatting applied in
provider_test.goand across the broader PR.providers/v1/aws/util/provider.go (1)
24-25: LGTM! Clean import organization.The import alias
awssmis clear and follows Go conventions. It's properly used in theSecretTagsToJSONStringfunction signature at line 72. This change aligns well with the PR's linting and formatting improvements.providers/v1/barbican/client.go (2)
34-42: Const block alignment is correct.The error constants are properly aligned for readability, consistent with Go formatting standards applied by golangci-lint. No issues here.
172-182: Function logic and edge case handling are correct.The
extractUUIDFromReffunction properly extracts the UUID from a Barbican secret reference URL with correct edge case handling (missing slash, trailing slash). The marked change at line 182 is purely a formatting adjustment with no functional impact.generators/v1/acr/acr.go (2)
245-252: LGTM! Formatting improves readability.The multiline function signature formatting makes the parameter list easier to read and maintain, especially for functions with multiple parameters.
333-340: LGTM! Consistent formatting applied.The multiline formatting is consistent with the other function signatures in this file and improves code readability.
providers/v1/infisical/provider.go (2)
30-30: LGTM! Import reordering improves organization.The import reordering follows Go conventions and improves code organization without any functional impact.
Also applies to: 33-33
78-85: LGTM! Function signature formatting enhances readability.The multi-line function signatures improve readability and align with Go formatting conventions for functions with multiple parameters. These formatting changes have no functional impact and are consistent with the PR's objective to enable linting and formatting improvements.
Also applies to: 107-114, 140-147, 164-171, 193-200, 222-229, 256-263, 322-329, 355-362, 379-386
providers/v1/beyondtrust/provider_test.go (1)
32-32: LGTM! Import reorganization aligns with linting objectives.The added imports and aliases are properly used throughout the file and follow Go conventions. These non-functional changes are appropriate for the linting and formatting improvements described in the PR objectives.
Also applies to: 34-34, 44-44
providers/v1/aws/parameterstore/parameterstore.go (1)
38-38: LGTM! Explicit import alias improves code clarity.The change from an implicit to an explicit
awsutilalias enhances readability and follows Go conventions for named imports. This aligns with the PR's objective to enable formatters and improve linting.providers/v1/aws/parameterstore/parameterstore_test.go (2)
38-39: LGTM! Import aliases improve consistency.The explicit import aliases align with the changes in the main file and enhance code readability. This is consistent with the PR's goal of enabling formatters.
517-519: LGTM! Multi-line formatting improves readability.The error construction has been reformatted across multiple lines, improving readability for the long error message. This is a formatting-only change with no functional impact, consistent with the PR's linting improvements.
providers/v1/volcengine/provider.go (1)
24-24: LGTM! Import organization follows Go conventions.The import placement correctly groups third-party packages and follows standard Go formatting.
pkg/controllers/pushsecret/pushsecret_controller.go (3)
47-47: LGTM: Improved import alias naming.The change from a generic import alias to
ctrlutilimproves code clarity and follows Go best practices for disambiguating imported packages.
394-400: LGTM: Multi-line formatting improves readability.The function signature formatting change improves readability without altering any functionality.
411-424: LGTM: Formatting and parameter passing improvements.The multi-line function signature improves readability. The direct construction of
storeRefusing separatestoreNameandrefKindparameters (lines 421-424) is cleaner than parsing composite strings, making the code more maintainable.providers/v1/oracle/oracle.go (2)
648-654: LGTM!The multiline function signature improves readability.
709-715: LGTM!The multiline formatting improves readability of the error construction.
main.go (1)
20-24: LGTM!The blank line separating the controller import from the side-effect register import is a reasonable formatting choice, distinguishing the functional import from the registration side-effect import.
apis/externalsecrets/v1/secretstore_barbican_types.go (1)
37-43: LGTM!The struct tag alignment is consistent and improves readability. No functional changes.
providers/v1/barbican/client_test.go (1)
204-206: LGTM!The indentation adjustment maintains correct test logic. The else-if branch properly handles unexpected errors that still contain the expected substring.
pkg/controllers/webhookconfig/webhookconfig.go (1)
40-43: LGTM!The import reordering correctly groups internal
external-secretsimports together. Theesutilspackage is properly imported and used at line 173.providers/v1/alibaba/kms_test.go (1)
28-32: LGTM!The import reordering correctly groups internal
external-secretsimports together. Theesutils.Ptr()utility is used consistently throughout the test file.providers/v1/beyondtrust/provider.go (1)
36-36: LGTM! Import reordering aligns with formatting standards.The import reordering is a formatting-only change that improves code consistency across the codebase.
providers/v1/cloudru/secretmanager/client.go (1)
35-35: LGTM! Import reordering for consistency.The import reordering is a formatting-only change that aligns with the project's formatting standards.
providers/v1/gitlab/gitlab_test.go (1)
260-263: LGTM! Multi-line formatting improves readability.The multi-line formatting of the append statement enhances readability while maintaining identical functionality.
providers/v1/azure/keyvault/keyvault_dual_sdk_test.go (1)
27-29: LGTM! Import grouping improves organization.The import reordering with blank line separation improves import block organization without affecting functionality.
providers/v1/barbican/fake/mock.go (1)
92-92: LGTM! Formatting adjustment for consistency.The formatting adjustment maintains identical functionality while aligning with code style standards.
apis/externalsecrets/v1/secretstore_validator_test.go (1)
121-125: LGTM! Multi-line formatting enhances test readability.The multi-line formatting of the assertion improves readability of the long error message while maintaining test functionality.
providers/v1/gcp/secretmanager/workload_identity_federation.go (1)
408-414: LGTM! Multi-line error formatting improves clarity.The multi-line formatting of the error message enhances readability without changing the error content or functionality.
providers/v1/chef/chef.go (1)
40-40: LGTM! Import reordering for consistency.The metrics import repositioning is a formatting-only change that maintains functionality while improving code organization.
pkg/controllers/crds/crds_controller.go (1)
49-51: LGTM!Import reorganization follows Go conventions by separating internal packages from external ones with a blank line.
providers/v1/kubernetes/metadata.go (1)
92-96: LGTM!Multi-line formatting improves readability for this function with multiple parameters.
providers/v1/onepassword/onepassword_test.go (1)
35-37: LGTM!Import reordering maintains consistent organization across the codebase.
providers/v1/aws/secretsmanager/secretsmanager.go (1)
45-49: LGTM!Import organization follows the project's formatting conventions.
providers/v1/azure/keyvault/fake/fake.go (1)
58-64: LGTM!Multi-line method signature formatting improves readability and aligns with the project's updated formatting standards.
providers/v1/chef/chef_test.go (1)
34-36: LGTM!Import reorganization follows the consistent pattern being applied across the codebase.
providers/v1/gcp/secretmanager/workload_identity_federation_test.go (1)
750-766: LGTM!Multi-line formatting for these long assertion statements improves code readability.
cmd/controller/certcontroller.go (1)
196-197: LGTM!Line break improves readability for this long flag registration statement.
generators/v1/gcr/gcr.go (1)
34-34: LGTM! Import reordering aligns with formatter configuration.The import reorganization is consistent with the PR's objective to enable formatters and has no functional impact.
providers/v1/yandex/common/provider.go (1)
37-37: LGTM! Import reordering aligns with formatter configuration.The import reorganization is consistent with the PR's objective to enable formatters and has no functional impact.
providers/v1/doppler/client.go (1)
34-37: LGTM! Import aliasing improves code organization.The addition of the
dclientalias and explicitfindimport enhances readability and aligns with the PR's formatting objectives. The aliases are used consistently throughout the file.providers/v1/doppler/provider.go (1)
33-33: LGTM! Import aliasing consistent with related files.The
dclientalias matches the pattern used inclient.goand aligns with the PR's formatting objectives.providers/v1/onboardbase/client.go (1)
35-35: LGTM! Import reordering with alias aligns with formatter configuration.The repositioning of the
obclientimport alias is consistent with the PR's formatting objectives and has no functional impact.providers/v1/onboardbase/provider.go (1)
28-29: LGTM! Import organization with alias improves consistency.The
oClientalias and import reordering align with the PR's formatting objectives and maintain consistency with related files.providers/v1/aws/util/errors_test.go (1)
32-40: LGTM! Test formatting improves readability.The multi-line formatting of error construction in test cases aligns with the PR's formatting objectives. The test logic and expected values remain unchanged.
apis/externalsecrets/v1beta1/secretstore_validator_test.go (1)
109-113: LGTM! Assertion formatting improves readability.The multi-line formatting of the assertion aligns with the PR's formatting objectives and makes the long error message more readable. The test logic remains unchanged.
apis/externalsecrets/v1/fakes/pushremoteref.go (1)
23-23: LGTM: Import alias improves clarity.The import alias clearly distinguishes the v1 package and is used consistently in the bottom-type assertion.
Also applies to: 130-130
generators/v1/vault/vault.go (1)
36-36: LGTM: Improved import alias and formatting.The rename from
utiltovaultutilmakes the import more descriptive and reduces ambiguity. The multi-line function signature improves readability without changing behavior.Also applies to: 74-81, 112-112
generators/v1/vault/vault_test.go (1)
35-35: LGTM: Test updates align with production code.The import alias change is consistent with the corresponding updates in
vault.go.Also applies to: 43-43
.golangci.yaml (1)
128-147: LGTM: Formatter configuration enables consistent code style.The enabled formatters (gci, gofmt, goimports, golines) with appropriate settings will help maintain consistent formatting across the codebase. The
max-len: 200for golines is generous but reasonable, and the gci section ordering follows Go conventions.cmd/controller/root.go (1)
359-360: LGTM: Formatting improves readability.The multi-line formatting of flag registrations improves readability without changing functionality.
Also applies to: 375-376
providers/v1/azure/keyvault/keyvault_test.go (1)
39-39: LGTM: Formatting-only changes to test fixtures.The multi-line formatting of base64-encoded test data improves readability. The static analysis warnings about private keys are expected and acceptable—these are test fixtures for certificate and key handling tests.
Also applies to: 392-807
providers/v1/aws/auth/auth.go (1)
38-38: LGTM: Import aliases and formatting improve code clarity.The import aliases (
ctrlcfg,awsutil) prevent naming collisions and improve readability. The multi-line function signatures make complex parameter lists more readable without changing behavior.Also applies to: 41-41, 179-187, 261-269
providers/v1/aws/auth/auth_test.go (1)
774-805: LGTM: Test formatting aligns with updated function signature.The multi-line formatting of the
NewGeneratorSessioncall improves readability and is consistent with the signature changes inauth.go.pkg/controllers/externalsecret/externalsecret_controller_secret.go (1)
155-161: LGTM - Consistent multiline function signature formatting.The reformatting of
handleGenerateSecrets,handleExtractSecrets, andhandleFindAllSecretsto multiline parameter lists with trailing commas improves readability and follows modern Go formatting conventions. No behavioral changes.Also applies to: 207-214, 255-262
Makefile (1)
149-183: Well-structured parallel linting implementation.The parallel execution approach using
xargs -Pwith result tracking via temp files is effective. A few observations:
- The
exit 1on line 170 won't stop other parallel jobs (which is correct behavior here for collecting all results).- The static analysis warning about body length is acceptable given the complexity of parallel execution with result aggregation.
One minor consideration: if a module's path contains special characters, the sed replacement on line 164 might produce unexpected filenames. This is unlikely in practice but worth noting.
.github/workflows/ci.yml (1)
85-88: LGTM - Consolidating lint into check-diff is a good simplification.The comment clearly explains the rationale. Since
make check-diff→reviewable→lint, running a separate lint job would be redundant. This reduces CI runtime and complexity.providers/v1/keepersecurity/client_test.go (1)
36-53: LGTM - Formatting improvements for readability.The constant alignment and multiline
fmt.Sprintfcalls for JSON construction improve code readability without changing behavior.Also applies to: 812-816, 836-841
providers/v1/aws/secretsmanager/secretsmanager_test.go (2)
46-47: LGTM - Import reorganization.The metadata import placement is appropriate.
1696-1758: The test filtering logic is correct and requires no changes.The mock's name filter logic may appear inverted, but it is never triggered in this test case. The production implementation in
findByNameAndTagsandfindByNameonly passes tag filters to ListSecrets, applying name regex matching entirely client-side viamatcher.MatchName(). The mock correctly processes the tag filters to return secrets with matching tags, and the production code then applies the client-side regex matching to produce the expected result.pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go (1)
374-380: LGTM - Consistent multiline function signature formatting.The reformatting improves readability and follows the same pattern applied to other controller functions in this PR.
providers/v1/vault/iamauth/iamauth.go (1)
164-172: LGTM - Consistent multiline function signature formatting.The reformatting of
CredsFromServiceAccountto a multiline parameter list improves readability, especially given the number of parameters. No behavioral changes.providers/v1/vault/client_push_test.go (1)
30-30: LGTM! Import alias improves consistency.The import alias
vaultutilfor the vault utility package improves code clarity and aligns with the broader refactoring across vault-related files in this PR.providers/v1/vault/auth_gcp_test.go (1)
28-30: LGTM! Import organization improved.The import block reordering with the added blank line follows Go conventions for separating standard/third-party imports from internal project imports.
providers/v1/vault/provider_test.go (1)
36-37: LGTM! Import aliases enhance code clarity.The addition of
vaultutilandutilfakeimport aliases prevents potential naming conflicts and improves code readability, consistent with the broader refactoring in this PR.generators/v1/ecr/resolver.go (1)
20-20: LGTM! Import consolidation improves organization.Consolidating the
contextimport into the main import block removes redundancy and improves code organization.providers/v1/conjur/client.go (1)
31-31: LGTM! Import alias enhances consistency.The
conjurutilimport alias improves code clarity and maintains consistency with similar aliasing patterns used for other provider utility packages in this PR.providers/v1/vault/client_get_all_secrets_test.go (1)
30-30: LGTM! Import alias maintains consistency.The
vaultutilimport alias aligns with the consistent aliasing pattern applied across all vault-related files in this PR, improving code organization.providers/v1/yandex/lockbox/client/grpcclient.go (1)
26-26: LGTM! Import alias improves clarity.The
ydxcommonimport alias prevents potential naming conflicts with generic "common" packages and makes the code more explicit about which common utilities are being used.providers/v1/vault/auth_iam.go (2)
63-71: LGTM! Multiline signature improves readability.The multiline function signature formatting makes the parameter list more readable and maintainable, especially helpful for functions with many parameters.
137-143: LGTM! Named argument formatting enhances clarity.The multiline formatting with named arguments for the
AWSAuthclient instantiation significantly improves code readability and makes it easier to understand the configuration being applied.providers/v1/vault/fake/vault.go (1)
29-29: LGTM! Import alias improves consistency.The
vaultutilimport alias aligns with the broader package-wide standardization of vault utility imports across this PR.providers/v1/yandex/certificatemanager/client/grpcclient.go (1)
26-26: LGTM! Import alias enhances consistency.The
ydxcommonimport alias standardizes references to Yandex common utilities across the provider, aligning with similar changes in other Yandex-related files.pkg/controllers/externalsecret/externalsecret_controller.go (1)
610-617: LGTM! Multi-line signature improves readability.The reformatted function signature makes the parameter list easier to scan and aligns with similar formatting improvements throughout the PR.
providers/v1/conjur/validate.go (1)
27-27: LGTM! Import alias improves package consistency.The
conjurutilimport alias standardizes references to Conjur utilities, matching the pattern applied to other provider packages in this PR.providers/v1/yandex/lockbox/client/fakeclient.go (1)
31-31: LGTM! Import alias and type reference updated consistently.The
ydxcommonimport alias and corresponding type reference update maintain consistency across the Yandex provider package.Also applies to: 141-141
providers/v1/vault/client_get_test.go (1)
33-33: LGTM! Test file import alias aligns with production code.The
vaultutilimport alias in the test file maintains consistency with the production code changes in other vault package files.providers/v1/vault/provider.go (1)
107-114: LGTM! Multi-line formatting enhances readability.The reformatted function signatures and flag registrations make the code easier to read and maintain, particularly for functions with multiple parameters.
Also applies to: 192-199, 328-340
providers/v1/vault/auth.go (1)
32-33: LGTM! Import consolidation improves consistency.The import aliases for
vaultiamauthandvaultutilconsolidate previously scattered imports and align with the broader standardization effort across the vault package.providers/v1/vault/client.go (1)
34-34: LGTM! Clean import aliasing.The introduction of the
vaultutilalias improves code clarity by making the package origin explicit throughout the file.providers/v1/yandex/lockbox/lockboxsecretgetter.go (1)
26-26: LGTM! Consistent aliasing and improved readability.The
ydxcommonalias and multi-line function signature formatting enhance code consistency and readability.Also applies to: 84-89
pkg/controllers/externalsecret/externalsecret_controller_test.go (4)
46-46: LGTM! Import alias improves clarity.The
ctrlutilalias provides clear indication of the utility package throughout the test file.
516-522: Excellent formatting improvement!Breaking the
fmt.Sprintfcall across multiple lines significantly improves readability of this complex string formatting operation.
942-944: Well-formatted assertion calls.The multi-line formatting of these
Expect().To()calls makes the test assertions easier to read and understand.Also applies to: 949-951
2330-2469: Much improved test table readability!The multi-line formatting of the
DescribeTableand its entries makes this extensive test suite significantly more maintainable. Each entry is now clearly separated and easier to scan.providers/v1/yandex/certificatemanager/client/fakeclient.go (1)
31-31: LGTM! Consistent import aliasing applied.The
ydxcommonalias is properly applied to the return type and usage, maintaining consistency across the Yandex provider codebase.Also applies to: 141-146
providers/v1/conjur/provider.go (1)
61-77: Excellent improvements to code clarity!The multi-line function signature enhances readability, and explicitly initializing all
Clientstruct fields (lines 69-76) improves code maintainability and makes the initialization intent clear.providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go (2)
25-28: LGTM! Clean import organization.The
ydxcommonalias and import grouping improve code organization and consistency with the rest of the Yandex provider implementation.
67-72: Well-formatted function signatures!The multi-line parameter formatting significantly improves readability, especially given the lengthy type names and parameter list.
Also applies to: 86-91
Skarlso
force-pushed
the
parallelize-the-lint
branch
from
10e3a91 to
2f3abaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/v1/oracle/oracle.go (1)
596-601: Error messages reference wrong environment variables (swapped).Line 596 unsets
ResourcePrincipalVersionEnvVarbut the error message incorrectly referencesResourcePrincipalRegionEnvVar. Conversely, line 600 unsetsResourcePrincipalRegionEnvVarbut referencesResourcePrincipalVersionEnvVar. Also, line 600 has a typo "unabled" → "unable" and uses a hardcoded string instead of theerrSettingOCIEnvVariablesconstant.🔎 Proposed fix
defer func() { if uerr := os.Unsetenv(auth.ResourcePrincipalVersionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalRegionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalVersionEnvVar, uerr)) } if uerr := os.Unsetenv(auth.ResourcePrincipalRegionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf("unabled to unset OCI SDK environment variable %s: %w", auth.ResourcePrincipalVersionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalRegionEnvVar, uerr)) } vms.workloadIdentityMutex.Unlock() }()
🧹 Nitpick comments (1)
providers/v1/oracle/oracle.go (1)
592-592: Typo in parameter name:serviceAcccountRefshould beserviceAccountRef.🔎 Proposed fix
func (vms *VaultManagementService) getWorkloadIdentityProvider( store esv1.GenericStore, - serviceAcccountRef *esmeta.ServiceAccountSelector, + serviceAccountRef *esmeta.ServiceAccountSelector, region, namespace string, ) (configurationProvider common.ConfigurationProvider, err error) {Note: Also update usages at lines 613, 617, and 628.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (102)
.github/workflows/ci.yml(1 hunks).github/workflows/publish.yml(1 hunks).github/workflows/release.yml(0 hunks).github/workflows/release_esoctl.yml(0 hunks).golangci.yaml(1 hunks)Makefile(1 hunks)apis/externalsecrets/v1/fakes/pushremoteref.go(1 hunks)apis/externalsecrets/v1/secretstore_barbican_types.go(1 hunks)apis/externalsecrets/v1/secretstore_validator_test.go(1 hunks)apis/externalsecrets/v1beta1/secretstore_validator_test.go(1 hunks)apis/generators/v1alpha1/types_mfa.go(1 hunks)cmd/controller/certcontroller.go(1 hunks)cmd/controller/root.go(2 hunks)generators/v1/acr/acr.go(2 hunks)generators/v1/cloudsmith/cloudsmith.go(0 hunks)generators/v1/ecr/resolver.go(1 hunks)generators/v1/fake/fake.go(0 hunks)generators/v1/gcr/gcr.go(1 hunks)generators/v1/github/github.go(0 hunks)generators/v1/grafana/grafana.go(0 hunks)generators/v1/mfa/mfa.go(0 hunks)generators/v1/quay/quay.go(0 hunks)generators/v1/sts/sts.go(0 hunks)generators/v1/uuid/uuid.go(0 hunks)generators/v1/vault/vault.go(2 hunks)generators/v1/vault/vault_test.go(1 hunks)main.go(1 hunks)pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go(1 hunks)pkg/controllers/crds/crds_controller.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller.go(1 hunks)pkg/controllers/externalsecret/externalsecret_controller_secret.go(3 hunks)pkg/controllers/externalsecret/externalsecret_controller_test.go(5 hunks)pkg/controllers/pushsecret/pushsecret_controller.go(3 hunks)pkg/controllers/secretstore/common_test.go(1 hunks)pkg/controllers/webhookconfig/webhookconfig.go(1 hunks)pkg/register/generators.go(0 hunks)providers/v1/akeyless/akeyless_test.go(1 hunks)providers/v1/alibaba/kms_test.go(1 hunks)providers/v1/aws/auth/auth.go(3 hunks)providers/v1/aws/auth/auth_test.go(1 hunks)providers/v1/aws/parameterstore/parameterstore.go(1 hunks)providers/v1/aws/parameterstore/parameterstore_test.go(2 hunks)providers/v1/aws/secretsmanager/secretsmanager.go(1 hunks)providers/v1/aws/secretsmanager/secretsmanager_test.go(3 hunks)providers/v1/aws/util/errors_test.go(1 hunks)providers/v1/aws/util/provider.go(1 hunks)providers/v1/azure/keyvault/fake/fake.go(1 hunks)providers/v1/azure/keyvault/keyvault_dual_sdk_test.go(1 hunks)providers/v1/azure/keyvault/keyvault_new_sdk.go(2 hunks)providers/v1/azure/keyvault/keyvault_test.go(9 hunks)providers/v1/barbican/client.go(2 hunks)providers/v1/barbican/client_test.go(1 hunks)providers/v1/barbican/fake/mock.go(1 hunks)providers/v1/barbican/provider.go(0 hunks)providers/v1/beyondtrust/provider.go(1 hunks)providers/v1/beyondtrust/provider_test.go(2 hunks)providers/v1/chef/chef.go(1 hunks)providers/v1/chef/chef_test.go(1 hunks)providers/v1/cloudru/secretmanager/client.go(1 hunks)providers/v1/cloudru/secretmanager/resolver.go(1 hunks)providers/v1/conjur/client.go(1 hunks)providers/v1/conjur/provider.go(1 hunks)providers/v1/conjur/validate.go(1 hunks)providers/v1/doppler/client.go(1 hunks)providers/v1/doppler/provider.go(1 hunks)providers/v1/gcp/secretmanager/workload_identity_federation.go(1 hunks)providers/v1/gcp/secretmanager/workload_identity_federation_test.go(1 hunks)providers/v1/gitlab/gitlab_test.go(1 hunks)providers/v1/ibm/fake/fake.go(1 hunks)providers/v1/infisical/client.go(2 hunks)providers/v1/infisical/provider.go(11 hunks)providers/v1/keepersecurity/client_test.go(3 hunks)providers/v1/kubernetes/client_test.go(2 hunks)providers/v1/kubernetes/metadata.go(1 hunks)providers/v1/ngrok/client_test.go(1 hunks)providers/v1/ngrok/provider.go(1 hunks)providers/v1/ngrok/provider_test.go(1 hunks)providers/v1/onboardbase/client.go(1 hunks)providers/v1/onboardbase/provider.go(1 hunks)providers/v1/onepassword/onepassword_test.go(1 hunks)providers/v1/oracle/oracle.go(3 hunks)providers/v1/previder/provider.go(1 hunks)providers/v1/senhasegura/auth/iso.go(1 hunks)providers/v1/vault/auth.go(1 hunks)providers/v1/vault/auth_gcp_test.go(1 hunks)providers/v1/vault/auth_iam.go(2 hunks)providers/v1/vault/client.go(1 hunks)providers/v1/vault/client_get_all_secrets_test.go(1 hunks)providers/v1/vault/client_get_test.go(1 hunks)providers/v1/vault/client_push_test.go(1 hunks)providers/v1/vault/fake/vault.go(1 hunks)providers/v1/vault/iamauth/iamauth.go(1 hunks)providers/v1/vault/provider.go(3 hunks)providers/v1/vault/provider_test.go(1 hunks)providers/v1/volcengine/provider.go(1 hunks)providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go(3 hunks)providers/v1/yandex/certificatemanager/client/fakeclient.go(1 hunks)providers/v1/yandex/certificatemanager/client/grpcclient.go(1 hunks)providers/v1/yandex/common/provider.go(1 hunks)providers/v1/yandex/lockbox/client/fakeclient.go(1 hunks)providers/v1/yandex/lockbox/client/grpcclient.go(1 hunks)providers/v1/yandex/lockbox/lockboxsecretgetter.go(2 hunks)
💤 Files with no reviewable changes (12)
- .github/workflows/release.yml
- generators/v1/sts/sts.go
- generators/v1/mfa/mfa.go
- .github/workflows/release_esoctl.yml
- generators/v1/grafana/grafana.go
- pkg/register/generators.go
- generators/v1/fake/fake.go
- generators/v1/github/github.go
- providers/v1/barbican/provider.go
- generators/v1/uuid/uuid.go
- generators/v1/cloudsmith/cloudsmith.go
- generators/v1/quay/quay.go
✅ Files skipped from review due to trivial changes (7)
- providers/v1/vault/auth_gcp_test.go
- providers/v1/ibm/fake/fake.go
- providers/v1/barbican/client_test.go
- providers/v1/akeyless/akeyless_test.go
- providers/v1/gcp/secretmanager/workload_identity_federation_test.go
- providers/v1/barbican/fake/mock.go
- providers/v1/aws/secretsmanager/secretsmanager_test.go
🚧 Files skipped from review as they are similar to previous changes (62)
- providers/v1/vault/client_get_test.go
- generators/v1/vault/vault_test.go
- providers/v1/ngrok/provider.go
- providers/v1/ngrok/provider_test.go
- providers/v1/yandex/certificatemanager/client/grpcclient.go
- providers/v1/azure/keyvault/keyvault_dual_sdk_test.go
- providers/v1/previder/provider.go
- providers/v1/aws/parameterstore/parameterstore_test.go
- apis/generators/v1alpha1/types_mfa.go
- providers/v1/aws/util/errors_test.go
- providers/v1/keepersecurity/client_test.go
- providers/v1/beyondtrust/provider_test.go
- providers/v1/aws/secretsmanager/secretsmanager.go
- pkg/controllers/secretstore/common_test.go
- providers/v1/aws/util/provider.go
- providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go
- providers/v1/vault/iamauth/iamauth.go
- providers/v1/conjur/provider.go
- providers/v1/yandex/lockbox/lockboxsecretgetter.go
- providers/v1/cloudru/secretmanager/client.go
- providers/v1/infisical/client.go
- providers/v1/vault/auth.go
- providers/v1/gitlab/gitlab_test.go
- providers/v1/conjur/client.go
- providers/v1/conjur/validate.go
- providers/v1/doppler/provider.go
- pkg/controllers/externalsecret/externalsecret_controller.go
- providers/v1/vault/client_get_all_secrets_test.go
- pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go
- providers/v1/yandex/lockbox/client/grpcclient.go
- providers/v1/yandex/common/provider.go
- providers/v1/aws/parameterstore/parameterstore.go
- providers/v1/alibaba/kms_test.go
- providers/v1/ngrok/client_test.go
- pkg/controllers/crds/crds_controller.go
- providers/v1/vault/fake/vault.go
- providers/v1/onboardbase/client.go
- providers/v1/volcengine/provider.go
- providers/v1/vault/provider_test.go
- cmd/controller/certcontroller.go
- providers/v1/chef/chef.go
- providers/v1/vault/client_push_test.go
- pkg/controllers/webhookconfig/webhookconfig.go
- .golangci.yaml
- generators/v1/ecr/resolver.go
- providers/v1/doppler/client.go
- providers/v1/vault/auth_iam.go
- apis/externalsecrets/v1beta1/secretstore_validator_test.go
- providers/v1/azure/keyvault/fake/fake.go
- providers/v1/senhasegura/auth/iso.go
- providers/v1/aws/auth/auth_test.go
- providers/v1/gcp/secretmanager/workload_identity_federation.go
- apis/externalsecrets/v1/fakes/pushremoteref.go
- cmd/controller/root.go
- generators/v1/acr/acr.go
- providers/v1/azure/keyvault/keyvault_new_sdk.go
- generators/v1/vault/vault.go
- pkg/controllers/pushsecret/pushsecret_controller.go
- apis/externalsecrets/v1/secretstore_barbican_types.go
- main.go
- generators/v1/gcr/gcr.go
- providers/v1/onboardbase/provider.go
🧰 Additional context used
🧬 Code graph analysis (3)
providers/v1/oracle/oracle.go (1)
apis/meta/v1/types.go (1)
ServiceAccountSelector(46-66)
pkg/controllers/externalsecret/externalsecret_controller_test.go (2)
apis/externalsecrets/v1/externalsecret_types.go (3)
AnnotationDataHash(686-686)LabelManaged(691-691)ExternalSecretReady(590-590)apis/externalsecrets/v1beta1/externalsecret_types.go (3)
AnnotationDataHash(581-581)LabelManaged(584-584)ExternalSecretReady(498-498)
pkg/controllers/externalsecret/externalsecret_controller_secret.go (2)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
Reconciler(137-153)apis/externalsecrets/v1beta1/externalsecret_types.go (2)
ExternalSecretDataFromRemoteRef(312-334)ExternalSecret(571-577)
🪛 checkmake (0.2.2)
Makefile
[warning] 149-149: Target body for "lint" exceeds allowed length of 5 (34).
(maxbodylength)
🪛 Gitleaks (8.30.0)
providers/v1/azure/keyvault/keyvault_test.go
[high] 402-402: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 615-615: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 630-630: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 789-789: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: Analyze project (go, autobuild)
- GitHub Check: Analyze project (actions, none)
🔇 Additional comments (33)
providers/v1/kubernetes/metadata.go (1)
92-96: LGTM! Clean formatting improvement.The function signature has been reformatted to a multi-line format, improving readability. This is a standard Go formatting convention and aligns with the PR's objective to enable formatters. No behavioral changes introduced.
apis/externalsecrets/v1/secretstore_validator_test.go (1)
121-125: LGTM! Formatting improves readability.The multi-line formatting of the
assert.EqualErrorcall makes the long expected error message easier to read and aligns with the PR's objective of enabling formatters.providers/v1/onepassword/onepassword_test.go (1)
36-36: LGTM! Import reordering aligns with formatting standards.The import statement reordering is a standard formatting change with no functional impact, consistent with the PR's linting and formatting objectives.
providers/v1/kubernetes/client_test.go (2)
592-592: LGTM! Formatting improvements align with PR objectives.The field alignment in struct literals improves code readability and is consistent with the linter/formatter configuration being enabled in this PR.
Also applies to: 599-599, 606-606
1184-1186: LGTM! Improved readability for long JSON strings.The multiline formatting of the lengthy JSON metadata string enhances readability while maintaining the exact same content. This change is consistent with the formatting improvements enabled by this PR.
pkg/controllers/externalsecret/externalsecret_controller_test.go (1)
46-46: LGTM! Formatting improvements enhance readability.The changes are purely cosmetic formatting improvements that align with enabling linters and formatters:
- Line 46: Import alias change (
util→ctrlutil) makes the package origin more explicit- Lines 516-522: Multi-line formatting of the
fmt.Sprintf+Equalassertion improves readability- Lines 942-951: Breaking
Expectcalls across multiple lines makes complex assertions easier to scan- Lines 2330-2469: Consistent formatting of
DescribeTableentries with trailing commas follows Go best practicesThese formatting changes improve maintainability without altering test behavior.
Also applies to: 516-522, 942-951, 2330-2469
providers/v1/yandex/certificatemanager/client/fakeclient.go (2)
31-31: LGTM! Import alias aligns with project conventions.The import alias
ydxcommonfor the common package is consistent with the pattern used across other Yandex provider files, improving code clarity and maintainability.
141-146: LGTM! Consistent usage of the import alias.The
ydxcommon.IamTokenreferences are correctly updated throughout the function, maintaining the same behavior while using the new import alias.providers/v1/cloudru/secretmanager/resolver.go (1)
25-28: LGTM!Import reordering to maintain alphabetical order within the internal package group aligns with Go conventions and the broader linting/formatting goals of this PR.
providers/v1/beyondtrust/provider.go (1)
36-36: LGTM! Import formatting aligns with linting improvements.The import reorganization is purely cosmetic and consistent with the PR's objective to enable formatters. The
v1alias is correctly used throughout the file.pkg/controllers/externalsecret/externalsecret_controller_secret.go (3)
155-161: LGTM! Improved signature formatting.The multiline parameter layout with trailing commas enhances readability and follows Go formatting best practices for functions with multiple parameters.
207-214: LGTM! Consistent signature formatting.The formatting is consistent with other refactored signatures in this file and improves code readability.
255-262: LGTM! Signature formatting complete.All three method signatures (
handleGenerateSecrets,handleExtractSecrets,handleFindAllSecrets) now follow a consistent multiline format, which improves code consistency and maintainability across the file.providers/v1/barbican/client.go (2)
35-41: LGTM! Formatting changes align with linting objectives.The alignment adjustments to the error constant declarations are consistent with the PR's goal of enabling formatters.
20-32: Inconsistency in AI summary regarding import removal.The AI summary states that the import of
gophercloud/openstack/keymanager/v1/secretswas removed from the import block, but line 28 shows this import is still present in the code (and correctly so, as it's used throughout the file in lines 57, 61, 66, 80, and 90).providers/v1/azure/keyvault/keyvault_test.go (2)
39-39: LGTM: Import path update for test mocks.The import path change to the fake package is appropriate for accessing mock implementations in tests.
392-403: LGTM: Multi-line formatting improves readability.The base64-encoded test data has been reformatted to use multi-line literals, which improves code readability without changing the decoded values. The static analysis warnings about "private keys" on lines 402, 615, 630, and 789 are false positives—these are test fixtures appropriate for test files, not actual production secrets.
Also applies to: 614-616, 629-631, 644-646, 771-773, 788-790, 805-807, 839-841
.github/workflows/ci.yml (2)
76-77: LGTM! Unconditional module download simplifies the workflow.Removing the cache-hit conditional ensures Go modules are always downloaded, which improves reliability and eliminates potential caching issues.
Also applies to: 110-111
86-87: Good clarification on lint execution.The comment clearly documents that
make check-diffnow handles linting, which justifies removing the separate lint job and reduces CI complexity.providers/v1/chef/chef_test.go (1)
35-35: LGTM! Import reordering improves consistency.This formatting change aligns with the project's import organization standards.
.github/workflows/publish.yml (1)
82-83: LGTM! Consistent with CI workflow changes.Unconditional module download ensures consistency across all workflows and improves reliability.
providers/v1/aws/auth/auth.go (2)
38-38: LGTM! Import aliases improve code clarity.The
ctrlcfgandawsutilaliases make the code more readable by avoiding conflicts and clearly indicating the package source.Also applies to: 41-41
179-187: LGTM! Multi-line signatures improve readability.Reformatting function signatures with many parameters to multi-line format significantly improves code readability and maintainability.
Also applies to: 261-269
Makefile (1)
149-183: Excellent improvement! Parallel linting will significantly speed up the workflow.The parallel execution using
xargs -Pwith per-module status tracking is well-implemented. The default of 20 parallel jobs is reasonable and can be overridden viaLINT_JOBS. The temporary directory approach for status tracking is clean and ensures proper cleanup.Note: The static analysis warning about body length exceeding 5 lines can be safely ignored here, as the complexity is necessary for proper parallel execution and result aggregation.
providers/v1/infisical/provider.go (2)
30-33: LGTM! Import organization improves consistency.The reordering of imports aligns with Go conventions and project standards.
78-85: LGTM! Multi-line signatures greatly improve readability.Reformatting these authentication functions with many parameters to multi-line format makes them much easier to read and maintain.
Also applies to: 107-114, 140-147, 164-171, 193-200, 222-229, 256-263, 322-329, 355-362, 379-386
providers/v1/vault/client.go (1)
34-34: LGTM! Import alias standardization improves consistency.Using the
vaultutilalias consistently across Vault-related files improves code organization and readability.providers/v1/vault/provider.go (2)
107-114: LGTM! Multi-line signatures enhance readability.Reformatting function signatures with many parameters to multi-line format significantly improves code clarity.
Also applies to: 192-199
328-340: LGTM! Multi-line flag registrations improve readability.Breaking the flag registration calls across multiple lines makes the flag definitions easier to read and understand.
providers/v1/oracle/oracle.go (2)
648-654: LGTM!Signature reflow for formatting compliance; no functional change.
709-715: LGTM!Multiline formatting for the
fmt.Errorfcall improves readability.providers/v1/yandex/lockbox/client/fakeclient.go (2)
31-31: LGTM: Import alias improves clarity.The addition of the
ydxcommonalias for the yandex common package is a clean refactoring that avoids potential naming conflicts and improves code readability.
141-145: LGTM: Consistent usage of import alias.The function signature and return statement correctly use the
ydxcommonalias throughout the codebase. Verification confirms that all 10 yandex provider files consistently import the common package with theydxcommonalias, with no unaliased imports or remainingcommon.IamTokenreferences. The implementation aligns with uniform naming conventions across both lockbox and certificatemanager providers.
Skarlso
force-pushed
the
parallelize-the-lint
branch
2 times, most recently
from
860640c to
db3e079
Compare
…rmatters Signed-off-by: Gergely Brautigam <[email protected]> On-behalf-of: Gergely Brautigam <[email protected]> Co-authored-by: Jakob Möller <[email protected]>
Skarlso
force-pushed
the
parallelize-the-lint
branch
from
db3e079 to
7375bc2
Compare
|
Now, this PR uses a tiny amount of parallelization because the runner actually can't really deal with it. And we don't have a correct cache setup so that doesn't help either. :) Locally it runs lint in SECONDS instead of MINUTES. So that's a huge improvement. Also removed the lint action which was double linting everything again. ( check-diff already runs the linter ). Also, turns out some of the formatters that we had code for actually were not enabled... So enabled those. |
moolen
left a comment
moolen
left a comment
There was a problem hiding this comment.
LGTM. 🍳 can fry an egg now on my computer with that many parallel lint jobs 😂
|
|
/ok-to-test sha=9a1d898c1890366a9d04ef0abea4bf61b8b01f80 |
|
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @hack/api-docs/Makefile:
- Line 25: The GITHUB_TOKEN default of "invalid" causes confusing git auth
errors when running make docs.publish; update the Makefile so the build.publish
target validates GITHUB_TOKEN before invoking mike deploy (or change the
default) by checking that the variable is set and not the placeholder "invalid"
and, if missing/invalid, print a clear error like "GITHUB_TOKEN is required to
publish docs; set GITHUB_TOKEN or run in CI" and exit non‑zero; reference the
GITHUB_TOKEN variable and the build.publish (make docs.publish) target when
implementing the pre-flight check.
In @providers/v1/barbican/fake/mock.go:
- Line 92: The slice `result` is incorrectly created with length 10
(make([]secrets.Secret, 10)), producing ten zero-valued entries so subsequent
appends (where `result` is appended later) end up after those empties; change
the initialization in `mock.go` to either an empty slice with capacity 10
(make([]secrets.Secret, 0, 10)) or nil/empty (var result []secrets.Secret) so
appends produce the intended elements, ensuring any code referencing `result`
(e.g., the append at the later block) behaves correctly.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/v1/oracle/oracle.go (1)
595-603: Critical: Fix swapped variable names and typo in error messages.The defer block contains critical bugs in the error handling:
Line 597: Error message references
auth.ResourcePrincipalRegionEnvVarbut should referenceauth.ResourcePrincipalVersionEnvVar(the variable being unset on line 596).Line 600: Error message references
auth.ResourcePrincipalVersionEnvVarbut should referenceauth.ResourcePrincipalRegionEnvVar(the variable being unset on line 599).Line 600: Typo "unabled" should be "unable".
Line 600: Inconsistent error format—should use the
errSettingOCIEnvVariablesconstant like line 597 for consistency.These bugs will cause incorrect error messages to be reported if unsetting environment variables fails.
🔎 Proposed fix
defer func() { if uerr := os.Unsetenv(auth.ResourcePrincipalVersionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalRegionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalVersionEnvVar, uerr)) } if uerr := os.Unsetenv(auth.ResourcePrincipalRegionEnvVar); uerr != nil { - err = errors.Join(err, fmt.Errorf("unabled to unset OCI SDK environment variable %s: %w", auth.ResourcePrincipalVersionEnvVar, uerr)) + err = errors.Join(err, fmt.Errorf(errSettingOCIEnvVariables, auth.ResourcePrincipalRegionEnvVar, uerr)) } vms.workloadIdentityMutex.Unlock() }()
🧹 Nitpick comments (1)
Makefile (1)
149-183: Consider documenting or reducing the default LINT_JOBS value.The parallel linting implementation is well-designed with proper error tracking and summary reporting. However, the default of 20 parallel jobs might be aggressive for local development environments.
Consider:
- Documenting
LINT_JOBSin the help text or README- Using a more conservative default (e.g., 4-8) for local development, since CI explicitly sets
LINT_JOBS=2The static analysis warning about body length is acceptable given the complexity required for parallel execution with proper error handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (103)
.github/workflows/ci.yml.github/workflows/publish.yml.github/workflows/release.yml.github/workflows/release_esoctl.yml.golangci.yamlMakefileapis/externalsecrets/v1/fakes/pushremoteref.goapis/externalsecrets/v1/secretstore_barbican_types.goapis/externalsecrets/v1/secretstore_validator_test.goapis/externalsecrets/v1beta1/secretstore_validator_test.goapis/generators/v1alpha1/types_mfa.gocmd/controller/certcontroller.gocmd/controller/root.gogenerators/v1/acr/acr.gogenerators/v1/cloudsmith/cloudsmith.gogenerators/v1/ecr/resolver.gogenerators/v1/fake/fake.gogenerators/v1/gcr/gcr.gogenerators/v1/github/github.gogenerators/v1/grafana/grafana.gogenerators/v1/mfa/mfa.gogenerators/v1/quay/quay.gogenerators/v1/sts/sts.gogenerators/v1/uuid/uuid.gogenerators/v1/vault/vault.gogenerators/v1/vault/vault_test.gohack/api-docs/Makefilemain.gopkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.gopkg/controllers/crds/crds_controller.gopkg/controllers/externalsecret/externalsecret_controller.gopkg/controllers/externalsecret/externalsecret_controller_secret.gopkg/controllers/externalsecret/externalsecret_controller_test.gopkg/controllers/pushsecret/pushsecret_controller.gopkg/controllers/secretstore/common_test.gopkg/controllers/webhookconfig/webhookconfig.gopkg/register/generators.goproviders/v1/akeyless/akeyless_test.goproviders/v1/alibaba/kms_test.goproviders/v1/aws/auth/auth.goproviders/v1/aws/auth/auth_test.goproviders/v1/aws/parameterstore/parameterstore.goproviders/v1/aws/parameterstore/parameterstore_test.goproviders/v1/aws/secretsmanager/secretsmanager.goproviders/v1/aws/secretsmanager/secretsmanager_test.goproviders/v1/aws/util/errors_test.goproviders/v1/aws/util/provider.goproviders/v1/azure/keyvault/fake/fake.goproviders/v1/azure/keyvault/keyvault_dual_sdk_test.goproviders/v1/azure/keyvault/keyvault_new_sdk.goproviders/v1/azure/keyvault/keyvault_test.goproviders/v1/barbican/client.goproviders/v1/barbican/client_test.goproviders/v1/barbican/fake/mock.goproviders/v1/barbican/provider.goproviders/v1/beyondtrust/provider.goproviders/v1/beyondtrust/provider_test.goproviders/v1/chef/chef.goproviders/v1/chef/chef_test.goproviders/v1/cloudru/secretmanager/client.goproviders/v1/cloudru/secretmanager/resolver.goproviders/v1/conjur/client.goproviders/v1/conjur/provider.goproviders/v1/conjur/validate.goproviders/v1/doppler/client.goproviders/v1/doppler/provider.goproviders/v1/gcp/secretmanager/workload_identity_federation.goproviders/v1/gcp/secretmanager/workload_identity_federation_test.goproviders/v1/gitlab/gitlab_test.goproviders/v1/ibm/fake/fake.goproviders/v1/infisical/client.goproviders/v1/infisical/provider.goproviders/v1/keepersecurity/client_test.goproviders/v1/kubernetes/client_test.goproviders/v1/kubernetes/metadata.goproviders/v1/ngrok/client_test.goproviders/v1/ngrok/provider.goproviders/v1/ngrok/provider_test.goproviders/v1/onboardbase/client.goproviders/v1/onboardbase/provider.goproviders/v1/onepassword/onepassword_test.goproviders/v1/oracle/oracle.goproviders/v1/previder/provider.goproviders/v1/senhasegura/auth/iso.goproviders/v1/vault/auth.goproviders/v1/vault/auth_gcp_test.goproviders/v1/vault/auth_iam.goproviders/v1/vault/client.goproviders/v1/vault/client_get_all_secrets_test.goproviders/v1/vault/client_get_test.goproviders/v1/vault/client_push_test.goproviders/v1/vault/fake/vault.goproviders/v1/vault/iamauth/iamauth.goproviders/v1/vault/provider.goproviders/v1/vault/provider_test.goproviders/v1/volcengine/provider.goproviders/v1/yandex/certificatemanager/certificatemanagersecretgetter.goproviders/v1/yandex/certificatemanager/client/fakeclient.goproviders/v1/yandex/certificatemanager/client/grpcclient.goproviders/v1/yandex/common/provider.goproviders/v1/yandex/lockbox/client/fakeclient.goproviders/v1/yandex/lockbox/client/grpcclient.goproviders/v1/yandex/lockbox/lockboxsecretgetter.go
💤 Files with no reviewable changes (12)
- .github/workflows/release.yml
- generators/v1/cloudsmith/cloudsmith.go
- generators/v1/mfa/mfa.go
- providers/v1/barbican/provider.go
- pkg/register/generators.go
- .github/workflows/release_esoctl.yml
- generators/v1/fake/fake.go
- generators/v1/github/github.go
- generators/v1/quay/quay.go
- generators/v1/uuid/uuid.go
- generators/v1/sts/sts.go
- generators/v1/grafana/grafana.go
✅ Files skipped from review due to trivial changes (9)
- providers/v1/alibaba/kms_test.go
- providers/v1/chef/chef_test.go
- providers/v1/yandex/common/provider.go
- providers/v1/vault/auth_iam.go
- providers/v1/vault/provider.go
- apis/externalsecrets/v1/secretstore_barbican_types.go
- providers/v1/akeyless/akeyless_test.go
- providers/v1/cloudru/secretmanager/client.go
- providers/v1/previder/provider.go
🚧 Files skipped from review as they are similar to previous changes (48)
- providers/v1/aws/parameterstore/parameterstore_test.go
- providers/v1/aws/secretsmanager/secretsmanager.go
- providers/v1/infisical/client.go
- pkg/controllers/crds/crds_controller.go
- providers/v1/azure/keyvault/keyvault_new_sdk.go
- providers/v1/aws/util/provider.go
- providers/v1/ngrok/provider_test.go
- pkg/controllers/secretstore/common_test.go
- providers/v1/chef/chef.go
- generators/v1/vault/vault.go
- providers/v1/onepassword/onepassword_test.go
- providers/v1/cloudru/secretmanager/resolver.go
- apis/externalsecrets/v1beta1/secretstore_validator_test.go
- providers/v1/gcp/secretmanager/workload_identity_federation.go
- providers/v1/keepersecurity/client_test.go
- providers/v1/barbican/client.go
- providers/v1/aws/auth/auth_test.go
- providers/v1/vault/auth_gcp_test.go
- providers/v1/senhasegura/auth/iso.go
- pkg/controllers/webhookconfig/webhookconfig.go
- providers/v1/ngrok/provider.go
- .golangci.yaml
- providers/v1/conjur/client.go
- providers/v1/vault/client_get_all_secrets_test.go
- cmd/controller/root.go
- providers/v1/beyondtrust/provider.go
- providers/v1/vault/client_push_test.go
- providers/v1/azure/keyvault/fake/fake.go
- providers/v1/doppler/client.go
- providers/v1/vault/provider_test.go
- apis/generators/v1alpha1/types_mfa.go
- providers/v1/aws/auth/auth.go
- providers/v1/aws/parameterstore/parameterstore.go
- providers/v1/yandex/lockbox/lockboxsecretgetter.go
- main.go
- providers/v1/kubernetes/metadata.go
- providers/v1/yandex/certificatemanager/certificatemanagersecretgetter.go
- providers/v1/volcengine/provider.go
- providers/v1/gitlab/gitlab_test.go
- providers/v1/onboardbase/client.go
- providers/v1/yandex/lockbox/client/grpcclient.go
- providers/v1/aws/secretsmanager/secretsmanager_test.go
- generators/v1/vault/vault_test.go
- providers/v1/ngrok/client_test.go
- providers/v1/yandex/certificatemanager/client/grpcclient.go
- providers/v1/gcp/secretmanager/workload_identity_federation_test.go
- providers/v1/conjur/provider.go
- pkg/controllers/pushsecret/pushsecret_controller.go
🧰 Additional context used
🧬 Code graph analysis (7)
providers/v1/barbican/fake/mock.go (1)
providers/v1/keepersecurity/client.go (1)
Secret(105-111)
pkg/controllers/externalsecret/externalsecret_controller_test.go (2)
apis/externalsecrets/v1beta1/externalsecret_types.go (3)
AnnotationDataHash(581-581)LabelManaged(584-584)ExternalSecretReady(498-498)apis/externalsecrets/v1/externalsecret_types.go (3)
AnnotationDataHash(686-686)LabelManaged(691-691)ExternalSecretReady(590-590)
providers/v1/oracle/oracle.go (1)
apis/meta/v1/types.go (1)
ServiceAccountSelector(46-66)
pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go (2)
pkg/controllers/externalsecret/externalsecret_controller.go (1)
Reconciler(138-154)apis/externalsecrets/v1beta1/clusterexternalsecret_types.go (2)
ClusterExternalSecret(122-128)ExternalSecretMetadata(61-67)
providers/v1/infisical/provider.go (1)
apis/externalsecrets/v1/secretsstore_infisical_types.go (1)
InfisicalProvider(156-178)
pkg/controllers/externalsecret/externalsecret_controller_secret.go (2)
apis/externalsecrets/v1beta1/externalsecret_types.go (2)
ExternalSecretDataFromRemoteRef(312-334)ExternalSecret(571-577)pkg/controllers/secretstore/client_manager.go (1)
Manager(50-58)
generators/v1/acr/acr.go (1)
apis/meta/v1/types.go (1)
ServiceAccountSelector(46-66)
🪛 checkmake (0.2.2)
Makefile
[warning] 149-149: Target body for "lint" exceeds allowed length of 5 (34).
(maxbodylength)
🪛 Gitleaks (8.30.0)
providers/v1/azure/keyvault/keyvault_test.go
[high] 402-402: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 615-615: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 630-630: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 789-789: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: unit-tests
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: Analyze project (actions, none)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (47)
apis/externalsecrets/v1/secretstore_validator_test.go (1)
121-125: LGTM!This is a straightforward formatting change that improves readability by breaking the long
assert.EqualErrorcall into multiple lines. Functionally equivalent to the original, and consistent with the PR's goal of enabling formatters.generators/v1/ecr/resolver.go (1)
19-28: LGTM! Import reorganization improves consistency.The
contextimport has been properly consolidated into the main import block, which aligns with standard Go formatting conventions. The import is correctly used in bothResolveEndpointmethod signatures.providers/v1/ibm/fake/fake.go (1)
47-50: LGTM! Signature formatting improves readability.The multi-line function signature formatting enhances readability without changing behavior, consistent with the PR's objective to enable formatters.
providers/v1/conjur/validate.go (2)
27-27: LGTM! Clean import alias.The import alias
conjurutilimproves code readability and aligns with Go naming conventions.
33-33: LGTM! Usage correctly updated.The function call properly uses the new import alias, maintaining consistency with the import declaration.
providers/v1/onboardbase/provider.go (1)
19-30: LGTM! Formatting-only change.The import reordering is purely cosmetic with no functional impact and aligns with the PR's objective of enabling formatters.
generators/v1/gcr/gcr.go (1)
34-34: LGTM! Import reordering from formatter.This import reordering is a cosmetic change with no functional impact, consistent with the PR's objective to enable formatters across the codebase.
providers/v1/aws/util/errors_test.go (1)
32-34: LGTM! Formatting improves readability.The multi-line formatting of these long error strings enhances readability without altering test behavior. This change aligns with the PR's objective of enabling formatters.
Also applies to: 38-40
apis/externalsecrets/v1/fakes/pushremoteref.go (2)
20-24: LGTM! Import alias improves code clarity.The explicit import alias for the v1 package is a standard Go practice that makes the code more maintainable and prevents potential naming conflicts.
130-130: LGTM! Type assertion correctly uses the import alias.The compile-time interface compliance check is a standard Go idiom that ensures this fake implementation maintains compatibility with the
v1.PushSecretRemoteRefinterface. The use of the aliased package name is consistent and improves code clarity.providers/v1/kubernetes/client_test.go (1)
592-592: LGTM! Formatting improvements enhance readability.The changes are purely cosmetic formatting adjustments:
- Consistent field alignment in struct literals (lines 592, 599, 606)
- Multi-line wrapping of long JSON metadata string (lines 1184-1186)
These formatting refinements improve code consistency and readability without any functional impact, which aligns perfectly with the PR's objective to enable formatters.
Also applies to: 599-599, 606-606, 1184-1186
cmd/controller/certcontroller.go (1)
196-197: LGTM! Formatting improvement for flag registration.The multi-line format improves readability for this long flag registration statement.
pkg/controllers/externalsecret/externalsecret_controller_test.go (4)
46-46: LGTM! Import alias added for consistency.The
ctrlutilalias is used consistently in the test file (e.g., lines 2499, 2523, 2563, etc.) and aligns with project-wide import conventions.
516-522: LGTM! Formatting improvement for readability.The multi-line format for the Equal assertion makes the long formatted string easier to read and maintain.
942-944: LGTM! Consistent formatting for test assertions.The multi-line format improves readability for assertions with long parameter lists.
Also applies to: 949-951
2330-2331: LGTM! Test table formatting improvements.The DescribeTable and Entry calls are reformatted to multi-line format with trailing commas, improving readability and making it easier to add or modify test cases.
Also applies to: 2426-2430, 2431-2435, 2436-2440, 2441-2445, 2446-2450, 2451-2455, 2457-2461, 2464-2468
pkg/controllers/externalsecret/externalsecret_controller.go (1)
611-618: LGTM! Method signature formatting improvement.The multi-line parameter list with trailing commas improves readability and is consistent with other formatting changes across the PR.
pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go (1)
374-380: LGTM! Method signature formatting improvement.The multi-line parameter list with trailing commas improves readability and aligns with the project-wide formatting standards applied in this PR.
pkg/controllers/externalsecret/externalsecret_controller_secret.go (1)
155-161: LGTM! Consistent method signature formatting.All three method signatures (handleGenerateSecrets, handleExtractSecrets, handleFindAllSecrets) are reformatted to multi-line format with trailing commas, improving readability and consistency across the codebase.
Also applies to: 207-214, 255-262
providers/v1/yandex/lockbox/client/fakeclient.go (2)
31-31: LGTM! Import alias improves clarity.The addition of the
ydxcommonalias for the yandex/common package makes the code more explicit and reduces potential naming conflicts with generic "common" package names. This aligns with the broader import aliasing refactor across Yandex provider files.
141-145: LGTM! Type references correctly updated.The references to
IamTokenhave been correctly updated to use the newydxcommonalias. The changes are consistent and maintain the existing functionality.providers/v1/yandex/certificatemanager/client/fakeclient.go (2)
31-31: LGTM! Consistent import aliasing.The import alias follows the same pattern as the lockbox client, maintaining consistency across the Yandex provider codebase. This improves code organization and readability.
141-145: LGTM! Type references correctly updated.The
IamTokentype references have been properly updated to use theydxcommonalias, consistent with the import change. No functional impact.providers/v1/vault/client_get_test.go (1)
33-33: LGTM! Import alias improves consistency.The vault util package import has been aliased to
vaultutilfor clarity, aligning with the broader import standardization across the vault provider package.providers/v1/vault/fake/vault.go (1)
29-29: LGTM! Consistent import aliasing.The import alias change to
vaultutilmaintains consistency with the vault provider package refactoring.providers/v1/vault/auth.go (1)
32-33: LGTM! Import organization improved.The vault IAM auth and util packages have been properly aliased and consolidated in the primary import block, improving code organization and consistency.
providers/v1/vault/client.go (2)
34-34: LGTM! Import aliasing standardized.The vault util package import has been aliased to
vaultutil, aligning with the package-wide import standardization effort.
46-49: LGTM! Type references updated correctly.The client struct fields have been properly updated to use the
vaultutilalias for Client, Auth, Logical, and Token types, maintaining type consistency with the import changes.providers/v1/vault/iamauth/iamauth.go (1)
164-172: LGTM! Function signature formatting improved.The
CredsFromServiceAccountsignature has been reformatted to multi-line with a trailing comma, improving readability and aligning with Go formatting conventions for functions with multiple parameters.providers/v1/azure/keyvault/keyvault_dual_sdk_test.go (1)
27-29: LGTM! Import reordering improves consistency.The import reordering aligns with Go conventions and improves code organization without any functional impact.
providers/v1/azure/keyvault/keyvault_test.go (2)
39-39: LGTM! Import path correctly updated.The import path change aligns with the test package structure for accessing mock/fake implementations.
392-841: LGTM! Test data refactoring improves readability.The extraction of large base64-encoded test fixtures into decoded variables significantly improves code readability by breaking up extremely long lines. The refactoring maintains functional equivalence while making the test setup clearer.
Note: The static analysis warnings about private keys are false positives—these are test fixtures that intentionally contain certificate and key data for testing purposes.
generators/v1/acr/acr.go (2)
245-252: LGTM: Formatting improvement for readability.The function signature has been reformatted to use multiline parameters, which improves readability for functions with multiple parameters. This change aligns with the PR's objective to enable formatters.
333-340: LGTM: Formatting improvement for readability.The function signature has been reformatted to use multiline parameters, consistent with the formatting applied throughout the PR.
providers/v1/oracle/oracle.go (3)
590-594: LGTM: Formatting improvement for readability.The function signature has been reformatted to use multiline parameters, which improves readability and aligns with the PR's objective to enable formatters.
648-654: LGTM: Formatting improvement for readability.The function signature has been reformatted to use multiline parameters, consistent with the formatting changes across the PR.
709-715: LGTM: Formatting improvement for readability.The
fmt.Errorfcall has been reformatted to use multiline arguments, which improves readability for complex error messages.providers/v1/infisical/provider.go (2)
30-33: LGTM: Import formatting improved.The import reordering aligns with standard Go formatting conventions and improves consistency across the codebase.
78-85: LGTM: Function signature formatting enhances readability.The multi-line parameter layout for these authentication functions significantly improves readability, making it easier to scan the parameter list for functions with multiple parameters. This is a standard Go formatting practice that aligns with the PR's objective of enabling formatters.
Also applies to: 107-114, 140-147, 164-171, 193-200, 222-229, 256-263, 322-329, 355-362, 379-386
providers/v1/doppler/provider.go (1)
33-33: LGTM! Import organization improved.The import deduplication and reordering is a good formatting improvement that aligns with the PR's objective of enabling formatters.
providers/v1/barbican/client_test.go (1)
205-205: LGTM! Formatting adjustment only.This indentation change aligns with the PR's linting objectives and has no functional impact.
providers/v1/beyondtrust/provider_test.go (1)
32-44: LGTM! Import organization aligns with linting standards.The import additions and reordering are consistent with the PR's linting/formatting objectives.
.github/workflows/publish.yml (1)
82-83: LGTM! Unconditional module download improves CI reliability.Removing the cache-hit conditional ensures Go modules are always available, preventing potential cache-related issues while still benefiting from Go's module cache when available.
.github/workflows/ci.yml (2)
82-86: LGTM! Conservative parallelization aligns with runner constraints.The
LINT_JOBS=2setting appropriately balances parallelization with GitHub runner limitations mentioned in the PR objectives. The consolidation of linting intocheck-diffsimplifies the workflow and reduces duplication.
73-74: LGTM! Unconditional module download improves reliability.Consistent with changes in other workflows, removing the conditional ensures modules are always available.
Makefile (2)
156-183: LGTM! Parallel linting implementation is robust.The implementation correctly:
- Uses a temporary directory with trap-based cleanup
- Tracks per-module success/failure
- Provides clear summary output
- Exits with appropriate error codes
- Supports both single-module and all-modules modes
290-301: LGTM! Documentation targets are well-structured.The docs targets provide clear separation between local serving, building, and publishing workflows.
| MINOR_VERSION ?= $(shell echo $(VERSION_NO_V) | cut -d'.' -f2) | ||
| VERSION_TO_CHECK ?= $(shell echo $(MAJOR_VERSION).$(MINOR_VERSION)) | ||
| DOCS_ALIAS ?= unstable | ||
| GITHUB_TOKEN ?= invalid |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how GITHUB_TOKEN is used in the documentation build scripts
rg -n -C3 'GITHUB_TOKEN' hack/api-docs/Repository: external-secrets/external-secrets
Length of output: 1559
🏁 Script executed:
#!/bin/bash
# Check mkdocs.yml and search for any documentation about mike/token usage
cat hack/api-docs/mkdocs.yml 2>/dev/null | head -100
# Also search for any comments or documentation in the Makefile around these lines
cat -n hack/api-docs/Makefile | sed -n '65,95p'
# Search for any documentation about mike usage
find . -name "*.md" -type f -exec rg -l "GITHUB_TOKEN|mike deploy" {} \; 2>/dev/null | head -10Repository: external-secrets/external-secrets
Length of output: 4954
🏁 Script executed:
#!/bin/bash
# Search for GitHub Actions workflows that might use these Makefile targets
find . -name "*.yml" -o -name "*.yaml" | xargs rg -l "api-docs|mike deploy" 2>/dev/null
# Check if there's a CI configuration that overrides GITHUB_TOKEN
rg -n "GITHUB_TOKEN" . --type yaml --type yml 2>/dev/null | head -20
# Look for documentation about running these targets
find . -name "*.md" -type f -exec rg -l "api-docs|Makefile.*build" {} \; 2>/dev/null | head -5Repository: external-secrets/external-secrets
Length of output: 221
🏁 Script executed:
#!/bin/bash
# List GitHub Actions workflow files
ls -la .github/workflows/ 2>/dev/null
# Check devguide for documentation about api-docs
rg -n -B2 -A5 "api-docs|Makefile" docs/contributing/devguide.md 2>/dev/null | head -50
# Search for any workflow files that use the build targets
rg -n "build\.publish|hack/api-docs" .github/workflows/ 2>/dev/nullRepository: external-secrets/external-secrets
Length of output: 2107
🏁 Script executed:
#!/bin/bash
# Check the docs.yml workflow
cat .github/workflows/docs.yml
# Check the publish.yml workflow
cat .github/workflows/publish.ymlRepository: external-secrets/external-secrets
Length of output: 6225
🏁 Script executed:
#!/bin/bash
# Check the complete Makefile for all docs-related targets
cat -n hack/api-docs/Makefile | grep -A15 "docs\."
# Check if there's a README in hack/api-docs
cat hack/api-docs/README.md 2>/dev/null
# Look for any documentation about mike's behavior with invalid tokens
rg -n "invalid|GITHUB_TOKEN" docs/contributing/devguide.md -B3 -A3Repository: external-secrets/external-secrets
Length of output: 2244
🏁 Script executed:
#!/bin/bash
# Search for any test files or documentation about mike
find . -type f \( -name "*test*" -o -name "*example*" \) -exec rg -l "mike\|GITHUB_TOKEN" {} \; 2>/dev/null | head -10
# Check the Dockerfile to understand the mkdocs/mike setup
cat hack/api-docs/Dockerfile 2>/dev/null
# Look for any comments or documentation about the --ignore flag
rg -n "ignore|--ignore" hack/api-docs/ -C2Repository: external-secrets/external-secrets
Length of output: 1667
🏁 Script executed:
#!/bin/bash
# Check requirements.txt to see what version of mike is used
cat hack/api-docs/requirements.txt
# Search for any GitHub API calls or token usage in the codebase
rg -n "GITHUB_TOKEN|github.com.*token" --type py hack/api-docs/ 2>/dev/null
# Look for any error handling or documentation about mike in comments
rg -n "mike\|deploy" hack/api-docs/*.sh 2>/dev/null | head -20Repository: external-secrets/external-secrets
Length of output: 914
🏁 Script executed:
#!/bin/bash
# Let's look at the git commit hash to understand the version
# Try to find any documentation in the repo about running docs.publish locally
rg -n "docs\.publish|GITHUB_TOKEN.*require\|token.*require" docs/ -i
# Check if there are any comments in the Makefile about token requirements
head -n 40 hack/api-docs/Makefile | tail -n 20
# Search for any GitHub issues or documentation references
find . -name "*.md" -exec rg -l "mike\|docs.publish" {} \; 2>/dev/null | xargs rg -B2 -A2 "GITHUB_TOKEN" 2>/dev/null | head -30Repository: external-secrets/external-secrets
Length of output: 1954
🏁 Script executed:
#!/bin/bash
# Let's verify the mike deploy behavior by checking the actual git commit
# and looking at how it handles invalid tokens
python3 << 'EOF'
import subprocess
import json
# The mike version uses a specific commit, let's check what --ignore does
result = subprocess.run(['pip', 'show', 'mike'], capture_output=True, text=True)
print("Mike package info:")
print(result.stdout)
# Check if there's any documentation in the installed package
result = subprocess.run(['python3', '-c', 'import mike; help(mike.deploy)'],
capture_output=True, text=True, timeout=5)
print("\nMike deploy help:")
print(result.stdout[:2000] if result.stdout else result.stderr[:2000])
EOFRepository: external-secrets/external-secrets
Length of output: 234
🌐 Web query:
mike deploy GITHUB_TOKEN invalid token behavior error handling
💡 Result:
Short answer
- If GITHUB_TOKEN is missing/invalid, mike's git push step fails with an authentication error (git cannot read password / authentication rejected) and mike prints the push failure (e.g. "error: failed to push branch gh-pages to origin" or "could not read Password for 'https://...':"). [1][2]
Why this happens
- mike itself commits to and pushes the docs branch via git; it relies on whatever Git auth is available in the environment. An invalid or absent token simply makes git fail to authenticate when pushing. [1][3]
How to detect (typical messages)
- "fatal: could not read Password for 'https://...':"
- "error: failed to push branch gh-pages to origin: ... ! [rejected] ... (fetch first / non-fast-forward)" (the latter can also appear when auth succeeded but the remote has new commits). These messages appear in CI logs where mike runs. [2][3]
Suggested fixes / error-handling steps
- Verify the token: ensure GITHUB_TOKEN (or a PAT) is present and correct in the environment/secrets. Use secrets.GITHUB_TOKEN in Actions or an exported PAT locally. [2][8]
- Ensure git will use the token: set the remote to include token (or use credential helper), e.g.
- remote_repo="https://x-access-token:${GITHUB_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" and add that as origin; or write ~/.git-credentials. This avoids interactive password prompts. [2][3]
- Ensure the token has the right permissions (for GitHub Actions use permissions: pages: write / contents: read as needed) so pushes to gh-pages are allowed. [6]
- If you get non-fast-forward rejections after fixing auth, fetch or rebase the remote gh-pages before deploy (git fetch origin gh-pages --depth=1) or allow mike to force/update according to your workflow. [1][3]
- For more robust CI: configure git user.name/email, fetch the gh-pages branch before deploy, and run mike deploy --push in the CI step. [1][3]
If you want, tell me whether you're running mike locally or inside GitHub Actions (and paste the exact error log) and I’ll give exact commands to fix it.
Sources
[1] mike README (deploy/pushing behavior).
[2] GitHub Actions / token authentication examples and common error discussion (StackOverflow / community posts).
[3] Example CI usage and remote setup showing token-in-remote pattern.
The "invalid" token default will cause confusing authentication failures when running make docs.publish locally.
The build.publish target passes GITHUB_TOKEN to mike deploy for pushing to the gh-pages branch. When the token is "invalid", git push will fail with a generic authentication error ("fatal: could not read Password for 'https://...'"), not a clear message about the missing/invalid token. This is confusing for local developers.
The serve target works fine without a token (it doesn't push), but users attempting make docs.publish will encounter vague git errors. While CI/CD is protected (the docs.yml workflow always provides secrets.GITHUB_TOKEN), improve the local development experience by either:
- Adding a pre-flight validation check that explicitly requires
GITHUB_TOKENforbuild.publish - Documenting that a valid
GITHUB_TOKENis required for deployment targets - Setting a more helpful default or error message
🤖 Prompt for AI Agents
In @hack/api-docs/Makefile around line 25, The GITHUB_TOKEN default of "invalid"
causes confusing git auth errors when running make docs.publish; update the
Makefile so the build.publish target validates GITHUB_TOKEN before invoking mike
deploy (or change the default) by checking that the variable is set and not the
placeholder "invalid" and, if missing/invalid, print a clear error like
"GITHUB_TOKEN is required to publish docs; set GITHUB_TOKEN or run in CI" and
exit non‑zero; reference the GITHUB_TOKEN variable and the build.publish (make
docs.publish) target when implementing the pre-flight check.
| } | ||
|
|
||
| var result = make([]secrets.Secret, 10) | ||
| var result = make([]secrets.Secret, 10) |
There was a problem hiding this comment.
Critical: Slice initialized with incorrect length.
The slice is initialized with length 10, not capacity 10. This creates 10 zero-valued secrets.Secret{} elements. When appending at line 102, new secrets are added after these 10 empty structs, breaking the mock's behavior.
🔎 Proposed fix
- var result = make([]secrets.Secret, 10)
+ var result = make([]secrets.Secret, 0, 10)Or simply:
- var result = make([]secrets.Secret, 10)
+ var result []secrets.Secret📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = make([]secrets.Secret, 10) | |
| var result = make([]secrets.Secret, 0, 10) |
🤖 Prompt for AI Agents
In @providers/v1/barbican/fake/mock.go around line 92, The slice `result` is
incorrectly created with length 10 (make([]secrets.Secret, 10)), producing ten
zero-valued entries so subsequent appends (where `result` is appended later) end
up after those empties; change the initialization in `mock.go` to either an
empty slice with capacity 10 (make([]secrets.Secret, 0, 10)) or nil/empty (var
result []secrets.Secret) so appends produce the intended elements, ensuring any
code referencing `result` (e.g., the append at the later block) behaves
correctly.
2 participants
Problem Statement
What is the problem you're trying to solve?
Related Issue
Fixes #...
Proposed Changes
How do you like to solve the issue and why?
Format
Please ensure that your PR follows the following format for the title:
Where
scopeis optionally one of:Checklist
git commit --signoffmake testmake reviewable