-
Notifications
You must be signed in to change notification settings - Fork 33
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
Conversation
Signed-off-by: Jared Watts <[email protected]>
There was a problem hiding this 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) |
There was a problem hiding this comment.
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:
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 |
There was a problem hiding this comment.
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:
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
.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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?
$(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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 😇
There was a problem hiding this comment.
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 <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
Let me know if there's anything else needed before merging @ulucinar @jeanduplessis 🎉 |
Description of your changes
This PR adds a new check to the
Makefile
that enforces theTERRAFORM_VERSION
cannot be set to something higher than1.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:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
I have run various
make
commands, such asmake reviewable
andmake
, 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
1.5.7
make
1.5.7
make reviewable
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.