Skip to content

chore: Simplifies makefile #2003

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

Merged
merged 3 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/acceptance-tests-runner.yml
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ on:
required: true

env:
TF_ACC: 1
Copy link
Member

Choose a reason for hiding this comment

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

while redundant I do see the value in leaving the TF_ACC env var just for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

don't know, if we assume "make testacc" does it i would remove it from here.

i would choose only one place to have it

Copy link
Collaborator

@oarbusi oarbusi Mar 11, 2024

Choose a reason for hiding this comment

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

[q] Does this mean that TF_ACC is not needed to run acceptance tests? Including locally? If so, we should update Terraform Development Guide

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 I see the value in keeping it

Copy link
Member Author

Choose a reason for hiding this comment

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

@oarbusi testacc makefile goal is: TF_ACC=1 go test ...

Copy link
Member Author

Choose a reason for hiding this comment

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

change reverted, i'll keep it, thx for the feedback

TF_LOG: ${{ vars.LOG_LEVEL }}
ACCTEST_TIMEOUT: ${{ vars.ACCTEST_TIMEOUT }}
MONGODB_ATLAS_BASE_URL: ${{ inputs.mongodb_atlas_base_url }}
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/migration-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ on:

env:
terraform_version: ${{ inputs.terraform_version || vars.TF_VERSION_LATEST }}
TF_ACC: 1
TF_LOG: ${{ vars.LOG_LEVEL }}
ACCTEST_TIMEOUT: ${{ vars.ACCTEST_TIMEOUT }}
MONGODB_ATLAS_BASE_URL: ${{ vars.MONGODB_ATLAS_BASE_URL }}
Expand Down
18 changes: 3 additions & 15 deletions GNUmakefile
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
TEST?=$$(go list ./...)
ACCTEST_TIMEOUT?=300m
PARALLEL_GO_TEST?=20
GOFMT_FILES?=$$(find . -name '*.go' |grep -v vendor)

BINARY_NAME=terraform-provider-mongodbatlas
DESTINATION=./bin/$(BINARY_NAME)

GOFLAGS=-mod=vendor
GOOPTS="-p 2"

GITTAG=$(shell git describe --always --tags)
VERSION=$(GITTAG:v%=%)
LINKER_FLAGS=-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=${VERSION}'
Expand All @@ -35,17 +31,17 @@ install: fmtcheck

.PHONY: test
test: fmtcheck
go test $(TEST) -timeout=30s -parallel=4 -race -covermode=atomic -coverprofile=coverage.out
go test ./... -timeout=30s -parallel=4 -race
Comment on lines -38 to +34
Copy link
Member

Choose a reason for hiding this comment

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

just for reference I see this was included originally for in #1496, but we no longer include these converage reports as PR comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, at the moment we don't include any coverage report


.PHONY: testacc
testacc: fmtcheck
@$(eval VERSION=acc)
TF_ACC=1 go test $(TEST) -run '$(TEST_REGEX)' -v -parallel '$(PARALLEL_GO_TEST)' $(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -cover -ldflags="$(LINKER_FLAGS)"
TF_ACC=1 go test ./... -run '$(TEST_REGEX)' -v -parallel '$(PARALLEL_GO_TEST)' $(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -ldflags="$(LINKER_FLAGS)"

.PHONY: testaccgov
testaccgov: fmtcheck
@$(eval VERSION=acc)
TF_ACC=1 go test $(TEST) -run 'TestAccProjectRSGovProject_CreateWithProjectOwner' -v -parallel 1 "$(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -cover -ldflags=$(LINKER_FLAGS) "
TF_ACC=1 go test ./... -run 'TestAccProjectRSGovProject_CreateWithProjectOwner' -v -parallel 1 "$(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -ldflags=$(LINKER_FLAGS) "

.PHONY: fmt
fmt:
Expand Down Expand Up @@ -81,13 +77,6 @@ tools: ## Install dev tools
go install github.com/hashicorp/terraform-plugin-codegen-framework/cmd/tfplugingen-framework@latest
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin $(GOLANGCI_VERSION)

.PHONY: check
check: test lint
Comment on lines -84 to -85
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we remove this? I would include additional check to this command such as fmtcheck

Copy link
Member Author

@lantoli lantoli Mar 11, 2024

Choose a reason for hiding this comment

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

it's not being used in GH actions or commit prechecks and i don't think anybody is using this

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah okay, I was probably the only one using it xD happy to remove it then


.PHONY: test-compile
test-compile:
go test -c $(TEST) $(TESTARGS)

.PHONY: website-lint
website-lint:
@echo "==> Checking website against linters..."
Expand Down Expand Up @@ -137,4 +126,3 @@ scaffold-schemas:
.PHONY: generate-doc
generate-doc: ## Generate the resource documentation via tfplugindocs
./scripts/generate-doc.sh ${resource_name}