Refactor constants from `internal/..` to `pkg/common` package. #3171

super-harsh · 2023-07-31T02:16:49Z

What this PR does / why we need it:

Refactoring constants from internal/.. to pkg/common package.

super-harsh · 2023-07-31T03:08:56Z

v2/pkg/common/constants.go

+const ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"
+
+const (
+	// ReconcilePolicyManage instructs the operator to manage the resource in question.


Should we keep these comments here, in reconcile_policy.go or both places?

Have to do it this way to avoid cyclic imports. Also, would be good to expose these values to external users with comments to have an idea of what the value does.

I think we move the ReconcilePolicy type here too, retaining type safety for clients who want it (and the others can cast it if they need to).

codecov-commenter · 2023-07-31T03:51:30Z

Codecov Report

Merging #3171 (f5354e6) into main (11f4803) will decrease coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is 66.00%.

@@            Coverage Diff             @@
##             main    #3171      +/-   ##
==========================================
- Coverage   54.38%   54.37%   -0.01%     
==========================================
  Files        1419     1420       +1     
  Lines      604182   604172      -10     
==========================================
- Hits       328557   328537      -20     
- Misses     221948   221954       +6     
- Partials    53677    53681       +4

Files Changed	Coverage Δ
v2/internal/testcommon/creds.go	`0.00% <0.00%> (ø)`
v2/internal/identity/credential_provider.go	`54.72% <38.46%> (ø)`
v2/internal/config/vars.go	`68.42% <78.57%> (ø)`
v2/internal/reconcilers/reconcile_policy.go	`89.47% <80.00%> (-1.84%)`	⬇️
...internal/reconcilers/generic/generic_reconciler.go	`83.89% <100.00%> (ø)`
v2/internal/reconcilers/predicates.go	`100.00% <100.00%> (ø)`
v2/pkg/common/reconciler/reconcile_policy.go	`100.00% <100.00%> (ø)`

... and 36 files with indirect coverage changes

theunrepentantgeek · 2023-08-01T01:54:03Z

v2/cmd/controller/app/setup.go

@@ -37,6 +37,7 @@ import (
 	. "github.com/Azure/azure-service-operator/v2/internal/logging"
 	asometrics "github.com/Azure/azure-service-operator/v2/internal/metrics"
 	armreconciler "github.com/Azure/azure-service-operator/v2/internal/reconcilers/arm"
+	"github.com/Azure/azure-service-operator/v2/pkg/common"


I'll beat @matthchr to this (because he always catches me out) - move this into the import group for local packages, just below.

theunrepentantgeek · 2023-08-01T01:56:05Z

v2/internal/reconcilers/arm/arm_client_cache_test.go

@@ -141,7 +141,7 @@ func Test_ARMClientCache_ReturnsPerResourceScopedClientOverNamespacedClient(t *t
 	g.Expect(err).ToNot(HaveOccurred())

 	rg := newResourceGroup("test-namespace")
-	rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: perResourceCredentialName.Name}
+	rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.Name}


I'd prefer if we spread these literal maps across multiple lines, as we already typically do elsewhere.

Suggested change

rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.Name}

rg.Annotations = map[string]string{

common.PerResourceSecretAnnotation: perResourceCredentialName.Name,

}

theunrepentantgeek · 2023-08-01T01:57:39Z

v2/internal/reconcilers/reconcile_policy.go

 type ReconcilePolicy string

 const (
 	// ReconcilePolicyManage instructs the operator to manage the resource in question.
-	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kuberentes.
+	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kubernetes.
 	// This is the default policy when no policy is specified.
-	ReconcilePolicyManage = ReconcilePolicy("manage")
+	ReconcilePolicyManage = ReconcilePolicy(common.ReconcilePolicyManage)

 	// ReconcilePolicySkip instructs the operator to skip all reconciliation actions. This includes creating
 	// the resource.
-	ReconcilePolicySkip = ReconcilePolicy("skip")
+	ReconcilePolicySkip = ReconcilePolicy(common.ReconcilePolicySkip)

 	// ReconcilePolicyDetachOnDelete instructs the operator to skip deletion of resources in Azure. This allows
 	// deletion of the resource in Kubernetes to go through but does not delete the underlying Azure resource.
-	ReconcilePolicyDetachOnDelete = ReconcilePolicy("detach-on-delete")
+	ReconcilePolicyDetachOnDelete = ReconcilePolicy(common.ReconcilePolicyDetachOnDelete)
 )



I suggest moving all of this into common - and get rid of the string typed constants. Consumers can then use the strongly typed ReconcilePolicyManage and cast it to a string if they must.

This also gets rid of the need to document them in two places.

Agree with @theunrepentantgeek

theunrepentantgeek · 2023-08-01T01:58:58Z

v2/pkg/common/constants.go

+	// #nosec
+	ClientSecretVar      = "AZURE_CLIENT_SECRET"
+	SubscriptionIDVar    = "AZURE_SUBSCRIPTION_ID"
+	TenantIDVar          = "AZURE_TENANT_ID"
+	ClientIDVar          = "AZURE_CLIENT_ID"
+	ClientCertificateVar = "AZURE_CLIENT_CERTIFICATE"
+	// #nosec
+	ClientCertificatePasswordVar = "AZURE_CLIENT_CERTIFICATE_PASSWORD"
+	TargetNamespacesVar          = "AZURE_TARGET_NAMESPACES"
+	OperatorModeVar              = "AZURE_OPERATOR_MODE"
+	SyncPeriodVar                = "AZURE_SYNC_PERIOD"
+	ResourceManagerEndpointVar   = "AZURE_RESOURCE_MANAGER_ENDPOINT"
+	ResourceManagerAudienceVar   = "AZURE_RESOURCE_MANAGER_AUDIENCE"
+	AzureAuthorityHostVar        = "AZURE_AUTHORITY_HOST"
+	PodNamespaceVar              = "POD_NAMESPACE"


We should remove the Var suffix from all of these, it's kinda implied.

Might pay to put a line or two of documentation on each one, maybe even a link to our docs.

theunrepentantgeek · 2023-08-01T01:59:46Z

v2/pkg/common/constants.go

+const ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"
+
+const (
+	// ReconcilePolicyManage instructs the operator to manage the resource in question.


I think we move the ReconcilePolicy type here too, retaining type safety for clients who want it (and the others can cast it if they need to).

matthchr · 2023-08-01T21:02:19Z

v2/internal/reconcilers/reconcile_policy.go

 type ReconcilePolicy string

 const (
 	// ReconcilePolicyManage instructs the operator to manage the resource in question.
-	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kuberentes.
+	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kubernetes.
 	// This is the default policy when no policy is specified.
-	ReconcilePolicyManage = ReconcilePolicy("manage")
+	ReconcilePolicyManage = ReconcilePolicy(common.ReconcilePolicyManage)

 	// ReconcilePolicySkip instructs the operator to skip all reconciliation actions. This includes creating
 	// the resource.
-	ReconcilePolicySkip = ReconcilePolicy("skip")
+	ReconcilePolicySkip = ReconcilePolicy(common.ReconcilePolicySkip)

 	// ReconcilePolicyDetachOnDelete instructs the operator to skip deletion of resources in Azure. This allows
 	// deletion of the resource in Kubernetes to go through but does not delete the underlying Azure resource.
-	ReconcilePolicyDetachOnDelete = ReconcilePolicy("detach-on-delete")
+	ReconcilePolicyDetachOnDelete = ReconcilePolicy(common.ReconcilePolicyDetachOnDelete)
 )



Agree with @theunrepentantgeek

matthchr · 2023-08-01T21:06:50Z

v2/pkg/common/constants.go

+
+const (
+	// #nosec
+	ClientSecretVar      = "AZURE_CLIENT_SECRET"


I think we should make some effort to differentiate what these constants are for. Previous, when they were in their own separate packages, it was "obvious" which ones were for the Configuration secret and which were used elsewhere.

Now, it's not so obvious. It will be even less obvious if we remove Var suffix from them (not that I disagree with the plan to remove that suffix).

I wonder if we should either put the config specific constants into a common subpackage, like common/config, so that it clearly explains what goes into the configuration secret, or else we could give them a Config suffix (I like that less). We could also at least put them into their own file, although that won't really help clarify for users who are using them as they don't see our file structure just our package structure.

All in all, I think I tend towards common/config

We could also have a common/annotations although I'm less certain that's a good idea.

I'm still not used to the way Go projects sometimes deliberately have very small packages purely for naming reasons - but I think both common/config and common/annotations have merit.

theunrepentantgeek · 2023-08-04T02:10:57Z

v2/internal/identity/credential_provider.go

@@ -95,7 +98,7 @@ func NewCredentialProvider(
 // If no matching credential can be found, an error is returned.
 func (c *credentialProvider) GetCredential(ctx context.Context, obj genruntime.MetaObject) (*Credential, error) {
 	// Resource annotation
-	cred, err := c.getCredentialFromAnnotation(ctx, obj, common.PerResourceSecretAnnotation)
+	cred, err := c.getCredentialFromAnnotation(ctx, obj, annotations.PerResourceSecretAnnotation)


Given the ~~namespace~~ package it's now in, can remove the suffix

Suggested change

cred, err := c.getCredentialFromAnnotation(ctx, obj, annotations.PerResourceSecretAnnotation)

cred, err := c.getCredentialFromAnnotation(ctx, obj, annotations.PerResourceSecret)

theunrepentantgeek · 2023-08-04T02:16:02Z

v2/pkg/common/annotations/annotations.go

+// A reconcile policy describes what action (if any) the operator is allowed to take when
+// reconciling the resource.
+// If no reconcile policy is specified, the default is "run"
+const ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"


Suffix mirrors the package name and can be removed.

Suggested change

const ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"

const ReconcilePolicy = "serviceoperator.azure.com/reconcile-policy"

Hmm actually, one interesting thing here is key vs value. This is the reconcile policy annotation key, but there's also a reconcile policy annotation value (which is currently in reconciler but IMO should be here.

We should adapt a common naming pattern for both of these.

annotations.ReconcilePolicy (for the annotation) and annotations.ReconcilePolicyValue (the enum type), and annotations.ReconcilePolicySkip or annotations.ReconcilePolicyManage for the actual values?

theunrepentantgeek · 2023-08-04T02:16:19Z

v2/pkg/common/config/config.go

+
+package config
+
+const (


These read really nicely now.

theunrepentantgeek · 2023-08-04T02:17:04Z

v2/pkg/common/reconciler/reconcile_policy.go

+	// ReconcilePolicyManage instructs the operator to manage the resource in question.
+	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kubernetes.
+	// This is the default policy when no policy is specified.
+	ReconcilePolicyManage = ReconcilePolicy("manage")


The common prefix here mirrors the name of the package, so can probably be removed for the type and all values.

Suggested change

ReconcilePolicyManage = ReconcilePolicy("manage")

PolicyManage = Policy("manage")

matthchr · 2023-08-04T19:12:55Z

v2/pkg/common/reconciler/reconcile_policy.go

+	// ReconcilePolicyManage instructs the operator to manage the resource in question.
+	// This includes issuing PUTs to update it and DELETE's to delete it from Azure if deleted in Kubernetes.
+	// This is the default policy when no policy is specified.
+	ReconcilePolicyManage = ReconcilePolicy("manage")


This is an annotation value, so shouldn't it just go in annotations?

matthchr · 2023-08-07T21:41:16Z

v2/pkg/common/annotations/annotations.go

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+package annotations


minor: Consider putting each annotation + value pair into its own file. So this could be called reconcile_policy.go

Refactor constants

3b44917

super-harsh self-assigned this Jul 31, 2023

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners July 31, 2023 02:16

Update comment

eb6244f

super-harsh changed the title ~~Refactor constants~~ Refactor constants from internal/.. to pkg/common package. Jul 31, 2023

super-harsh added 2 commits July 31, 2023 14:25

Add header

492e474

Get public facing constants for reconcile policy

16c386e

super-harsh commented Jul 31, 2023

View reviewed changes

Update test const references

bf3c765

theunrepentantgeek approved these changes Aug 1, 2023

View reviewed changes

matthchr reviewed Aug 1, 2023

View reviewed changes

super-harsh added 4 commits August 2, 2023 19:30

Move common constants to respective packages

a2e9c6d

fix ref

f5354e6

Merge branch 'main' into refactor/constants

c5b2607

Merge branch 'main' into refactor/constants

0a0a467

theunrepentantgeek approved these changes Aug 4, 2023

View reviewed changes

matthchr reviewed Aug 4, 2023

View reviewed changes

super-harsh added 2 commits August 8, 2023 09:38

Refactor to move reconcilePolicy to common.annotations

41ee7b3

Merge branch 'main' into refactor/constants

caa6da8

matthchr approved these changes Aug 7, 2023

View reviewed changes

Move reconcilePolicy annotations to reconcile_policy.go

8b044bc

super-harsh enabled auto-merge (squash) August 7, 2023 22:12

super-harsh merged commit 6bd3b0d into main Aug 7, 2023

super-harsh deleted the refactor/constants branch August 7, 2023 22:43

nojnhuh mentioned this pull request Aug 29, 2023

Add pod identity support for namespaces and per-resource scoped auth #3187

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor constants from `internal/..` to `pkg/common` package. #3171

Refactor constants from `internal/..` to `pkg/common` package. #3171

super-harsh commented Jul 31, 2023

super-harsh Jul 31, 2023

super-harsh Jul 31, 2023

theunrepentantgeek Aug 1, 2023

codecov-commenter commented Jul 31, 2023 •

edited

Loading

theunrepentantgeek Aug 1, 2023

theunrepentantgeek Aug 1, 2023

theunrepentantgeek Aug 1, 2023

theunrepentantgeek Aug 1, 2023

matthchr Aug 1, 2023

theunrepentantgeek Aug 1, 2023

theunrepentantgeek Aug 1, 2023

matthchr Aug 1, 2023

matthchr Aug 1, 2023

matthchr Aug 1, 2023

theunrepentantgeek Aug 1, 2023

theunrepentantgeek Aug 4, 2023

theunrepentantgeek Aug 4, 2023

matthchr Aug 7, 2023

theunrepentantgeek Aug 4, 2023

theunrepentantgeek Aug 4, 2023

matthchr Aug 4, 2023

matthchr Aug 7, 2023

	cred, err := c.getCredentialFromAnnotation(ctx, obj, annotations.PerResourceSecretAnnotation)
	cred, err := c.getCredentialFromAnnotation(ctx, obj, annotations.PerResourceSecret)

	const ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"
	const ReconcilePolicy = "serviceoperator.azure.com/reconcile-policy"

	ReconcilePolicyManage = ReconcilePolicy("manage")
	PolicyManage = Policy("manage")

Refactor constants from internal/.. to pkg/common package. #3171

Refactor constants from internal/.. to pkg/common package. #3171

Conversation

super-harsh commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor constants from `internal/..` to `pkg/common` package. #3171

Refactor constants from `internal/..` to `pkg/common` package. #3171

codecov-commenter commented Jul 31, 2023 •

edited

Loading