Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update 'images list' cmd to use config variables, not hardcoded values #3690

Merged
merged 9 commits into from
Jul 24, 2024

Conversation

busma13
Copy link
Contributor

@busma13 busma13 commented Jul 23, 2024

This PR makes the following changes:

  • Add several new variables to config.ts:
    • KIND_DOCKER_IMAGE
    • KIND_VERSION
    • BASE_DOCKER_IMAGE
    • TEST_NODE_VERSIONS
    • DEFAULT_NODE_VERSION
    • __DEFAULT_ELASTICSEARCH6_VERSION
    • __DEFAULT_ELASTICSEARCH7_VERSION
    • __DEFAULT_OPENSEARCH1_VERSION
    • __DEFAULT_OPENSEARCH2_VERSION
  • images list command creates the image tags and versions based off the values in config.ts instead of hard-coded values.
  • Replace other hard-coded values where necessary
  • Remove version flags from test-runner and k8s-env commands and get all versions from config.ts

@busma13 busma13 marked this pull request as ready for review July 23, 2024 17:28
@busma13 busma13 requested review from godber and sotojn July 23, 2024 18:05
@busma13 busma13 force-pushed the docker-image-version-variables branch from e3e840a to 4acb1b4 Compare July 24, 2024 16:44
@busma13 busma13 force-pushed the docker-image-version-variables branch from a99cc78 to eea51a8 Compare July 24, 2024 20:57
@busma13 busma13 changed the title update images list cmd to use config variables, not hardcoded values Update 'images list' cmd to use config variables, not hardcoded values Jul 24, 2024
Copy link
Member

@godber godber left a comment

Choose a reason for hiding this comment

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

I am going to approve this PR but I REALLY dislike the use of environment variables and would like to push for us to STOP using them except were absolutely necessary (like passing stuff to jest). The reason I dislike it is that they can be abused and used as global variables. This PR does not abuse that and all of the uses are confined to config.ts and then properties on config. This is OK. But if people start using process.env to access these values all over the place we will start shooting ourselves in the foot.

@godber godber merged commit b4bcf7e into master Jul 24, 2024
62 checks passed
@godber godber deleted the docker-image-version-variables branch July 24, 2024 23:22
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.

2 participants