Skip to content

Align all env variables for cluster management in all languages#147

Merged
vic-tsang merged 7 commits intomainfrom
consistent_variable_names
May 20, 2025
Merged

Align all env variables for cluster management in all languages#147
vic-tsang merged 7 commits intomainfrom
consistent_variable_names

Conversation

@vic-tsang
Copy link
Copy Markdown
Contributor

  • renamed all env variables to use the standard ones

Single Cluster

  • CLUSTER_REGION
  • CLUSTER_ID

Multi Clusters

  • CLUSTER_1_REGION
  • CLUSTER_2_REGION
  • WITNESS_REGION
  • CLUSTER_1_ID
  • CLUSTER_2_ID

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT-0 license.

Thank you for your contribution!

@vic-tsang vic-tsang requested a review from danielfrankcom May 17, 2025 01:24
Copy link
Copy Markdown
Contributor

@danielfrankcom danielfrankcom left a comment

Choose a reason for hiding this comment

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

Changes here mostly look good.

I did a quick search in the branch and found the following which I think were missed:

  • go/cluster_management/cmd/update_cluster/update_cluster_integ_test.go
			region:     os.Getenv("REGION"),
			identifier: os.Getenv("CLUSTER_ID"),
  • go/cluster_management/cmd/get_cluster/get_cluster_integ_test.go
			region:     os.Getenv("REGION"),
			identifier: os.Getenv("CLUSTER_ID"),

For whatever reason it seems these are also setting environment variables but any change to that is likely out of scope. We should use consistent names imo. For extra weirdness points, the create_multi_region_integ_test.go file and the single-region equivalent don't use environment variables at all. Even the ones I mentioned above inconsistently reference the region, sometimes with an env var and sometimes hardcoded.

Comment thread python/cluster_management/src/delete_single_region.py Outdated
Comment thread python/cluster_management/src/get_cluster.py Outdated
Comment thread python/cluster_management/src/update_cluster.py Outdated
Comment thread python/cluster_management/README.md Outdated
Comment thread python/cluster_management/test/test_dsql_cluster_management.py
@vic-tsang vic-tsang force-pushed the consistent_variable_names branch from aa72b5b to c0aebf7 Compare May 20, 2025 21:02
Co-authored-by: Daniel Frankcom <frankcom@amazon.com>
@danielfrankcom danielfrankcom changed the title Align all env variables for cluster management in all langugages Align all env variables for cluster management in all languages May 20, 2025
@vic-tsang vic-tsang merged commit 63d7fe8 into main May 20, 2025
27 of 35 checks passed
@vic-tsang vic-tsang deleted the consistent_variable_names branch May 20, 2025 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants