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

Updates to GNUmakefile and .travis.yml #21

Merged
merged 15 commits into from
May 30, 2019
Merged

Updates to GNUmakefile and .travis.yml #21

merged 15 commits into from
May 30, 2019

Conversation

jakewan
Copy link
Contributor

@jakewan jakewan commented May 28, 2019

This PR updates GNUmakefile and .travis.yml as described by #14. Also includes some changes to emulate Google and AWS providers.

Testing should minimally include calling make testacc.

make lint works in the container now too, but you'll need to run make docker-build again and start over with a fresh container (i.e. docker rm citrixitm_tf_dev_container, if necessary, before make docker-run).

@jakewan jakewan requested review from wraithan and aggarwaa May 28, 2019 22:43
@@ -1,23 +1,24 @@
dist: trusty
dist: xenial
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AWS has updated to Xenial.

sudo: required
services:
- docker
language: go
go:
- "1.9.x"
- "1.12.x"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AWS uses 1.12.x

@@ -1,4 +1,4 @@
FROM golang:1.11.6-alpine3.9
FROM golang:1.12.5-alpine3.9
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using go 1.12.x in the Travis VM, so might as well update our Docker environment to use it too.

@@ -8,6 +8,10 @@ RUN apk add --update \
shadow \
vim

# Additional go tools
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are needed for the new lint make target.

@@ -102,4 +102,4 @@ else
@docker run -it --name $(DOCKER_CONTAINER_NAME) --env ITM_BASE_URL --env ITM_CLIENT_ID --env ITM_CLIENT_SECRET --mount type=bind,src=$(PWD),dst=$(DOCKER_SOURCE_DIR) --mount type=bind,src=$(ITM_HOST_MODULE_DIR),dst=/terraform-module $(DOCKER_IMAGE_NAME) /bin/bash
endif

.PHONY: build test testacc vet fmt fmtcheck errcheck vendor-status test-compile website website-test
.PHONY: build fmt fmtcheck install-tools lint test test-compile testacc website website-lint website-test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will keep these dependencies alphabetized from now on.

@sh -c "'$(CURDIR)/scripts/gofmtcheck.sh'"

errcheck:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, nothing uses this.

errcheck:
@sh -c "'$(CURDIR)/scripts/errcheck.sh'"

vendor-status:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed per Hashicorp.

echo "Vet found suspicious constructs. Please check the reported constructs"; \
echo "and fix them if necessary before submitting the code for review."; \
exit 1; \
fi

Choose a reason for hiding this comment

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

does the lint check basically make this target redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it appears that most of the big providers have replaced the vet target with the lint one.

@jakewan jakewan merged commit bcf35f7 into master May 30, 2019
@jakewan jakewan deleted the jacob/gh-issue-14 branch May 30, 2019 18:28
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