Add information to header #3992

benjaminjb · 2024-09-13T17:36:08Z

Checklist:

Have you added an explanation of what your changes do and why you'd like them to be included?
Have you updated or added documentation for the change, as applicable?
Have you tested your changes on all related environments with successful results, as applicable?
- Have you added automated tests?

Type of Changes:

What is the current behavior (link to any open issues here)?

What is the new behavior (if this is a feature change)?

Breaking change (fix or feature that would cause existing functionality to change)

Other Information:
Issues: [PGO-1610, PGO-1616, PGO-1618]

internal/registration/runner_test.go

internal/upgradecheck/header_test.go

internal/upgradecheck/helpers_test.go

internal/upgradecheck/http_test.go

dsessler7

LGTM

cbandy · 2024-09-17T16:20:31Z

internal/upgradecheck/header_test.go

+		saveEnv(t, "PGO_FEATURE_GATES")
+		os.Setenv("PGO_FEATURE_GATES", "Tablespace=true")


🔧 It looks like ctx is wired through from CheckForUpgradesScheduler.Start. If so,¹ this test can use the feature package like so:

Suggested change

saveEnv(t, "PGO_FEATURE_GATES")

os.Setenv("PGO_FEATURE_GATES", "Tablespace=true")

gate := feature.NewGate()

assert.NilError(t, gate.SetFromMap(map[string]bool{

feature.TablespaceVolumes: true,

}))

ctx = feature.NewContext(ctx, gate)

Footnotes

main.go initializes that base/root context. ↩

cbandy · 2024-09-17T16:24:02Z

internal/upgradecheck/helpers_test.go

+
+// saveEnv preserves environment variables so that any modifications needed for
+// the tests can be undone once completed.
+func saveEnv(t testing.TB, key string) {


🔧 Use T.Setenv instead.

Pulled that out of all the tests.

(There's no t.Unsetenv, but someone suggested using t.Setenv to save the env, and then calling os.Unsetenv, so I tried that where applicable)

cbandy · 2024-09-17T16:26:07Z

internal/upgradecheck/http_test.go

+	saveEnv(t, "PGO_FEATURE_GATES")
+	os.Setenv("PGO_FEATURE_GATES", "Tablespace=true")


⛏️ I can't tell; is there a ctx around here? If so, use that instead.

internal/upgradecheck/http.go

cbandy · 2024-09-17T18:13:39Z

internal/registration/runner.go

@@ -86,7 +87,7 @@ func (r *Runner) CheckToken() error {
 	r.token.Lock()
 	defer r.token.Unlock()

-	_, errToken := jwt.ParseWithClaims(string(data), &r.token, key,
+	token, errToken := jwt.ParseWithClaims(string(data), &r.token, key,


🌱 Passing &r.token here doesn't seem right according to the docs, but handling custom claims through an interface... 🤔 🤔

📝 🦆 🤔 IIUC, the other package needs only a plaintext opaque blob of "token."

dsessler7 · 2024-10-01T00:17:30Z

internal/upgradecheck/header.go

+	BridgeClustersTotal int    `json:"bridge_clusters_total"`
+	DeploymentID        string `json:"deployment_id"`
+	FeatureGatesEnabled string `json:"feature_gates_enabled"`
+	IsOpenShift         bool   `json:"is_open_shift"`
+	KubernetesEnv       string `json:"kubernetes_env"`
+	PGOClustersTotal    int    `json:"pgo_clusters_total"`
+	PGOVersion          string `json:"pgo_version"`
+	RegistrationToken   string `json:"registration_token"`
+	PGOInstaller        string `json:"pgo_installer"`
+	PGOInstallerOrigin  string `json:"pgo_installer_origin"`
+	BuildSource         string `json:"build_source"`


⛏️ We should probably alphabetize these struct fields.

dsessler7 · 2024-10-01T00:18:22Z

internal/upgradecheck/header.go

+		BridgeClustersTotal: getBridgeClusters(ctx, crClient),
+		FeatureGatesEnabled: feature.ShowGates(ctx),
+		IsOpenShift:         isOpenShift,
+		DeploymentID:        ensureDeploymentID(ctx, crClient),
+		KubernetesEnv:       getServerVersion(ctx, cfg),
+		PGOClustersTotal:    getManagedClusters(ctx, crClient),
+		PGOVersion:          pgoVersion,
+		RegistrationToken:   registrationToken,
+		PGOInstaller:        os.Getenv("PGO_INSTALLER"),
+		PGOInstallerOrigin:  os.Getenv("PGO_INSTALLER_ORIGIN"),
+		BuildSource:         os.Getenv("BUILD_SOURCE"),


⛏️ We should probably alphabetize these struct fields.

dsessler7 · 2024-10-01T00:40:20Z

internal/upgradecheck/http.go

+	OpenShift         bool
+	Refresh           time.Duration
+	URL, Version      string
+	RegistrationToken string


⛏️ We should probably alphabetize these.

dsessler7 · 2024-10-01T00:40:31Z

internal/upgradecheck/http.go

+		Client:            m.GetClient(),
+		Config:            m.GetConfig(),
+		OpenShift:         openshift,
+		Refresh:           24 * time.Hour,
+		URL:               url,
+		Version:           version,
+		RegistrationToken: token,


⛏️ We should probably alphabetize these.

tjmoore4 · 2024-10-01T19:05:32Z

internal/feature/features.go

@@ -121,3 +121,12 @@ func Enabled(ctx context.Context, f Feature) bool {
 func NewContext(ctx context.Context, gate Gate) context.Context {
 	return context.WithValue(ctx, contextKey{}, gate)
 }
+
+func ShowGates(ctx context.Context) string {


⛏️ Since this is a public function, I'd like a comment.

* Crunchy Bridge clusters managed * Features gates enabled * Registration token * Build metadata Issues: [PGO-1610, PGO-1616, PGO-1618]

benjaminjb requested review from cbandy, crunchyandrew and dsessler7 and removed request for crunchyandrew September 13, 2024 19:27

dsessler7 reviewed Sep 16, 2024

View reviewed changes

dsessler7 approved these changes Sep 16, 2024

View reviewed changes

cbandy reviewed Sep 17, 2024

View reviewed changes

dsessler7 approved these changes Sep 20, 2024

View reviewed changes

dsessler7 approved these changes Oct 1, 2024

View reviewed changes

tjmoore4 approved these changes Oct 2, 2024

View reviewed changes

Add fields to header

049a777

* Crunchy Bridge clusters managed * Features gates enabled * Registration token * Build metadata Issues: [PGO-1610, PGO-1616, PGO-1618]

benjaminjb force-pushed the update-header branch from c83f96b to 049a777 Compare October 3, 2024 01:15

benjaminjb merged commit 6707a99 into master Oct 3, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add information to header #3992

Add information to header #3992

benjaminjb commented Sep 13, 2024

dsessler7 left a comment

cbandy Sep 17, 2024

benjaminjb Sep 18, 2024

cbandy Sep 17, 2024

benjaminjb Sep 18, 2024 •

edited

Loading

cbandy Sep 17, 2024

benjaminjb Sep 18, 2024

cbandy Sep 17, 2024

dsessler7 Oct 1, 2024

dsessler7 Oct 1, 2024

dsessler7 Oct 1, 2024

dsessler7 Oct 1, 2024

tjmoore4 Oct 1, 2024

		saveEnv(t, "PGO_FEATURE_GATES")
		os.Setenv("PGO_FEATURE_GATES", "Tablespace=true")

-		saveEnv(t, "PGO_FEATURE_GATES")
-		os.Setenv("PGO_FEATURE_GATES", "Tablespace=true")
+		gate := feature.NewGate()
+		assert.NilError(t, gate.SetFromMap(map[string]bool{
+			feature.TablespaceVolumes: true,
+		}))
+		ctx = feature.NewContext(ctx, gate)

Add information to header #3992

Add information to header #3992

Conversation

benjaminjb commented Sep 13, 2024

dsessler7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminjb Sep 18, 2024 • edited Loading

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

benjaminjb Sep 18, 2024 •

edited

Loading