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

build: add check to enforce max allowed terraform version #67

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

jbw976
Copy link
Member

@jbw976 jbw976 commented Jun 3, 2024

Description of your changes

This PR adds a new check to the Makefile that enforces the TERRAFORM_VERSION cannot be set to something higher than 1.5.x. Starting with v1.6, Terraform started being licensed under the BSL, which is not a permitted license for the Crossplane project.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I have run various make commands, such as make reviewable and make, with various test values and observed the behavior in the table below. Versions below 1.6 are built successfully and versions greater than or equal to 1.6 will fail and stop the build.

TERRAFORM_VERSION command result
1.5.7 make OK
1.5.7 make reviewable OK
1.6.0 make Makefile:111: *** invalid TERRAFORM_VERSION 1.6.0, must be less than 1.6. Stop.
1.8.4 make reviewable Makefile:111: *** invalid TERRAFORM_VERSION 1.8.4, must be less than 1.6. Stop.

Signed-off-by: Jared Watts <jbw976@gmail.com>
Makefile Show resolved Hide resolved
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @jbw976, lgtm. Left some further points for us to discuss but I'm completely fine merging this as is.

Makefile Outdated
# Do not allow a version of terraform greater than 1.5.x, due to versions 1.6+ being
# licensed under BSL, which is not permitted.
export TERRAFORM_VERSION_CEILING ?= 1.6
TERRAFORM_VERSION_VALID := $(shell [ $(TERRAFORM_VERSION) = `echo "$(TERRAFORM_VERSION)\n$(TERRAFORM_VERSION_CEILING)" | sort -V | head -n1` ] && echo 1 || echo 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:
Not critical as these are already expected to be version strings but to prevent globbing & word splitting, we may consider using double quotes:

Suggested change
TERRAFORM_VERSION_VALID := $(shell [ $(TERRAFORM_VERSION) = `echo "$(TERRAFORM_VERSION)\n$(TERRAFORM_VERSION_CEILING)" | sort -V | head -n1` ] && echo 1 || echo 0)
TERRAFORM_VERSION_VALID := $(shell [ "$(TERRAFORM_VERSION)" = "`echo "$(TERRAFORM_VERSION)\n$(TERRAFORM_VERSION_CEILING)" | sort -V | head -n1`" ] && echo 1 || echo 0)

Makefile Outdated

# Do not allow a version of terraform greater than 1.5.x, due to versions 1.6+ being
# licensed under BSL, which is not permitted.
export TERRAFORM_VERSION_CEILING ?= 1.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think it makes sense not to allow overriding the ceiling from the environment:

Suggested change
export TERRAFORM_VERSION_CEILING ?= 1.6
export TERRAFORM_VERSION_CEILING := 1.6

At least, this will further prevent something like TERRAFORM_VERSION_CEILING=1.7 TERRAFORM_VERSION=1.6.0 make build.

If you like this idea and consider this as a valid concern, I will even suggest not defining the ceiling in a variable and instead hardcoding the string literal 1.6 below as we will have also covered make command-line variables, i.e., the case make TERRAFORM_VERSION_CEILING=1.7 build.

Copy link
Member Author

Choose a reason for hiding this comment

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

great idea @ulucinar - I removed this from being a variable at all and have hardcoded this value into the makefile.

Makefile Outdated
$(TERRAFORM):
check-terraform-version:
ifneq ($(TERRAFORM_VERSION_VALID),1)
$(error invalid TERRAFORM_VERSION $(TERRAFORM_VERSION), must be less than $(TERRAFORM_VERSION_CEILING))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should we mention about the licensing concerns here?

Suggested change
$(error invalid TERRAFORM_VERSION $(TERRAFORM_VERSION), must be less than $(TERRAFORM_VERSION_CEILING))
$(error invalid TERRAFORM_VERSION $(TERRAFORM_VERSION), must be less than $(TERRAFORM_VERSION_CEILING) as versions starting with 1.6.0 use BSL)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have expanded the message here to make it visibly clear why this version check exists when this error is shown.

Makefile Outdated
# Do not allow a version of terraform greater than 1.5.x, due to versions 1.6+ being
# licensed under BSL, which is not permitted.
export TERRAFORM_VERSION_CEILING ?= 1.6
TERRAFORM_VERSION_VALID := $(shell [ $(TERRAFORM_VERSION) = `echo "$(TERRAFORM_VERSION)\n$(TERRAFORM_VERSION_CEILING)" | sort -V | head -n1` ] && echo 1 || echo 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

also a bit worried about the portability of using control chars (\n) with echo - that isn't allowed as per https://www.shellcheck.net/

^-- [SC2028](https://www.shellcheck.net/wiki/SC2028) (info): echo may not expand escape sequences. Use printf.

Will look into using printf for this check instead 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

i have incorporated printf now instead of using echo and have tested the same scenarios are still working.

Signed-off-by: Jared Watts <jbw976@gmail.com>
Copy link
Member Author

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback @ulucinar! I have incorporated all of your feedback 🙇‍♂️

Makefile Outdated

# Do not allow a version of terraform greater than 1.5.x, due to versions 1.6+ being
# licensed under BSL, which is not permitted.
export TERRAFORM_VERSION_CEILING ?= 1.6
Copy link
Member Author

Choose a reason for hiding this comment

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

great idea @ulucinar - I removed this from being a variable at all and have hardcoded this value into the makefile.

Makefile Outdated
# Do not allow a version of terraform greater than 1.5.x, due to versions 1.6+ being
# licensed under BSL, which is not permitted.
export TERRAFORM_VERSION_CEILING ?= 1.6
TERRAFORM_VERSION_VALID := $(shell [ $(TERRAFORM_VERSION) = `echo "$(TERRAFORM_VERSION)\n$(TERRAFORM_VERSION_CEILING)" | sort -V | head -n1` ] && echo 1 || echo 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

i have incorporated printf now instead of using echo and have tested the same scenarios are still working.

Makefile Outdated
$(TERRAFORM):
check-terraform-version:
ifneq ($(TERRAFORM_VERSION_VALID),1)
$(error invalid TERRAFORM_VERSION $(TERRAFORM_VERSION), must be less than $(TERRAFORM_VERSION_CEILING))
Copy link
Member Author

Choose a reason for hiding this comment

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

I have expanded the message here to make it visibly clear why this version check exists when this error is shown.

@jbw976
Copy link
Member Author

jbw976 commented Jun 4, 2024

Let me know if there's anything else needed before merging @ulucinar @jeanduplessis 🎉

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.

None yet

3 participants