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.

Sign up for GitHub

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

Jump to bottom

chore: Creates mock generation infrastructure #1763

Merged
merged 27 commits into from
Dec 19, 2023
Merged

chore: Creates mock generation infrastructure #1763

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
add48d9
use mockery
lantoli Dec 17, 2023
b41ba5e
don't include test utilities in build
lantoli Dec 17, 2023
cc90022
Revert "don't include test utilities in build"
lantoli Dec 17, 2023
0cbacbc
TEMPORARY: testing mock change detection
lantoli Dec 17, 2023
630cd1b
Revert "TEMPORARY: testing mock change detection"
lantoli Dec 17, 2023
bb449fb
centralize lint version in make file
lantoli Dec 17, 2023
fee2a7e
fix call to mockery
lantoli Dec 17, 2023
c76d184
timeout in linter
lantoli Dec 17, 2023
d171e9b
TEMPORARY - delete mock file
lantoli Dec 17, 2023
a3dd559
Revert "TEMPORARY - delete mock file"
lantoli Dec 17, 2023
194a843
use assert
lantoli Dec 17, 2023
05ed39b
TEMPORARY - failing unit test
lantoli Dec 17, 2023
481fd10
Revert "TEMPORARY - failing unit test"
lantoli Dec 17, 2023
9520446
use testify mock
lantoli Dec 17, 2023
92abb8f
change service name
lantoli Dec 17, 2023
19e3159
use state
lantoli Dec 17, 2023
8907cd8
disable version mockery generated files
lantoli Dec 17, 2023
b761422
refactor response
lantoli Dec 17, 2023
a125d08
responses in one line each
lantoli Dec 17, 2023
5b7f3ad
rename err
lantoli Dec 17, 2023
7d1595d
clarify file detection message
lantoli Dec 18, 2023
4b4066b
add info in contributing file
lantoli Dec 18, 2023
3b478f7
move timeout to config file
lantoli Dec 18, 2023
5ef6390
TEMPORARY - linter fail
lantoli Dec 19, 2023
233ce19
Revert "TEMPORARY - linter fail"
lantoli Dec 19, 2023
7191cd1
remove asdf comment
lantoli Dec 19, 2023
650b772
revert lint changes
lantoli Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 24 additions & 29 deletions .github/workflows/code-health.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,34 @@ jobs:
runs-on: ubuntu-latest
marcosuma marked this conversation as resolved.
Show resolved Hide resolved
steps:
AgustinBettati marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/checkout@v4
- name: Set up Go
uses: actions/setup-go@v5
- uses: actions/setup-go@v5
lantoli marked this conversation as resolved.
Show resolved Hide resolved
with:
go-version-file: 'go.mod'
- name: Install tools for mocks and linter
run: make tools
- name: Mock generation
run: mockery
- name: Check for uncommited files
Copy link
Member Author

@lantoli lantoli Dec 17, 2023 •
edited

Choose a reason for hiding this comment

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

taken from Atlas CLI.
We don't run mockery in each make build so it doesn't get slow, but we detect if it was not run when pushing the commits.

run: |
export FILES=$(git ls-files -o -m --directory --exclude-standard --no-empty-directory)
export LINES=$(echo "$FILES" | awk 'NF' | wc -l)
if [ $LINES -ne 0 ]; then
echo "Detected files that need to be committed:"
echo "$FILES" | sed -e "s/^/ /"
echo ""
echo "Mock skeletons are not up-to-date, you may have forgotten to run mockery before committing your changes."
exit 1
fi
- name: Build
run: make build
- name: Linter
run: make lint
Copy link
Member Author

Choose a reason for hiding this comment

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

linter here takes 2m instead of 3m and linter version is centralized in make file

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure how it was before 🤔 I don't understand the advantages of adding these changes, maybe you could help me understand here. Could you clarify the current state without your changes and what your changes are trying to achieve? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

the main advantage about the linter change is that we'll have the version only in one place, now it's in 2 places and we need to remember every time we upgrade it:
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/GNUmakefile#L16
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/.github/workflows/code-health.yml#L72

Copy link
Member Author

@lantoli lantoli Dec 18, 2023 •
edited

Choose a reason for hiding this comment

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

with this change we use make lint from GH action so version is only in one place

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see, thanks. I thought this was related somehow to the mock test but it is not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI by moving out of the golangci lint action to the make file version you are loosing the action ability to comment on the line with the failure and you gonna have to parse the log files to figure out what broke

Copy link
Member Author

Choose a reason for hiding this comment

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

@gssbzn that's fair but we already have a pre-commit hook with linter so linter should rarely fail in GH

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gus made an interesting comment on that. And I guess your answer seems more like "this is a duplicate" - so my point is that either it's useful having linter here (hence we need to make the results easily actionable, hence this does not seem a good idea) or it's not.

Since it's not actually related to the change you are proposing in the PR I would suggest to remove this change the things as it is. Your pros mentioned above to Andrea as well does not look that strong to me compared to Gus's comment.

Copy link
Member Author

@lantoli lantoli Dec 19, 2023 •
edited

Choose a reason for hiding this comment

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

I pushed an offending commit (bypasing commit hooks) with current linter and the one in this PR to compare

Current linter: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7258950919/job/19775168073

Running [/home/runner/golangci-lint-1.55.0-linux-amd64/golangci-lint run --out-format=github-actions --timeout 10m] in [] ...
  Error: test helper function should start from tb.Helper() (thelper)
  
  Error: issues found
  Ran golangci-lint in 169508ms

Linter in this PR: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7259036317/job/19775411159?pr=1763

golangci-lint run
Error: internal/testutil/acc/skip.go:18:[6](https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7259036317/job/19775411159?pr=1763#step:8:7): test helper function should start from tb.Helper() (thelper)
func SkipIfTFAccNotDefined(tb testing.TB) {
     ^
make: *** [GNUmakefile:68: lint] Error 1
Error: Process completed with exit code 2.

So apart from having the linter version only in one place, here we have information about the offending file and line whereas in the current approach we only see the error not where it happens.

Please can you clarify why you think the current situation is better? Also I'm not sure I understand the part "the action ability to comment on the line with the failure ". thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2023-12-19 at 13 59 33

I am not gonna try to answer on behalf of Gus, but I just have a question: would we see this linter error on GH if we move the linter in this new place?

unit-test:
needs: build
runs-on: ubuntu-latest
permissions:
pull-requests: write
Comment on lines -28 to -29
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we weakening the permissions?

Copy link
Member Author

@lantoli lantoli Dec 18, 2023 •
edited

Choose a reason for hiding this comment

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

@gssbzn the intention was to simplify the action, what problems or risks do you see removing this? (I'm not sure why we're defining permissions in this job but not in others)

Copy link
Collaborator

Choose a reason for hiding this comment

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

at some point we'll have to do some audit of gh actions permissions and be sure GH actions only use what they need, I'm not sure when was this permission added or why, a blame to understand the context would be great before trying to change it

I started some of this auditing on the CLI using https://github.com/ossf/scorecard but it's quite consuming and I haven't had the time to dig deeper on 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.

i see, thanks for the info, it looks like we'll need to a focused effort on this and change all workflows.

steps:
- name: Checkout
uses: actions/checkout@v4
- name: Set up Go
uses: actions/setup-go@v5
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
- name: Unit Test
Expand All @@ -56,28 +68,11 @@ jobs:
with:
recreate: true
path: code-coverage-results.md
lint:
Copy link
Member Author

Choose a reason for hiding this comment

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

lint done in build to centralize version in make file

runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Install Go
uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
cache: false # see https://github.com/golangci/golangci-lint-action/issues/807
- name: golangci-lint
uses: golangci/golangci-lint-action@v3.7.0
with:
version: v1.55.0
args: --timeout 10m
Copy link
Member Author

Choose a reason for hiding this comment

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

timeout is in config file now

website-lint:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Install Go
uses: actions/setup-go@v5
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
- name: website lint
Expand All @@ -89,11 +84,11 @@ jobs:
- name: Run ShellCheck
uses: bewuethr/shellcheck-action@v2
call-acceptance-tests-workflow:
needs: [build, lint, shellcheck, unit-test, website-lint]
needs: [build, shellcheck, unit-test, website-lint]
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you remove lint here? if the lint fails we should not run the 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.

the lint is now running inside build, so threre is still a dependency on it

secrets: inherit
uses: ./.github/workflows/acceptance-tests.yml
call-migration-tests-workflow:
needs: [build, lint, shellcheck, unit-test, website-lint]
needs: [build, shellcheck, unit-test, website-lint]
secrets: inherit
uses: ./.github/workflows/migration-tests.yml

3 changes: 1 addition & 2 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,8 @@ issues:
text: "^hugeParam: req is heavy"

run:
timeout: 10m
tests: true
build-tags:
- integration
skip-dirs:
- internal/mocks
modules-download-mode: readonly
11 changes: 11 additions & 0 deletions .mockery.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
with-expecter: false
Copy link
Member Author

@lantoli lantoli Dec 17, 2023 •
edited

Choose a reason for hiding this comment

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

go generate syntax is deprecated in favor of mockery config file

disable-version-string: true
dir: internal/testutil/mocksvc
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] since your interfaces are not centralized don't centralize the mocks, it leads to a lot of hooks if an interface happens to share a name and also to a lot of issues later on when you refactor your interfaces

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. I wanted to start simple, it's very easy to change at any moment to have mocks in their resource folders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just be vigilant this doesn't become the permanent state, I'm currently trying to de-tangle some mock mess in the CLI and collocation would've made this easier

outpkg: mocksvc
filename: "{{ .InterfaceName | snakecase }}.go"
mockname: "{{.InterfaceName}}"

packages:
? github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment
: interfaces:
DeploymentService:
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ To do this you can:
- Helper methods must have their own tests, e.g. `common_advanced_cluster_test.go` has tests for `common_advanced_cluster.go`.
- `internal/testutils/acc` contains helper test methods for Acceptance and Migration tests.
- Tests that need the provider binary like End-to-End tests don’t belong to the source code packages and go in `test/e2e`.
- [Testify Mock](https://pkg.go.dev/github.com/stretchr/testify/mock) and [Mockery](https://github.com/vektra/mockery) are used for test doubles in unit tests. Mocked interfaces are generated in folder `internal/testutil/mocksvc`.


### Creating New Resource and Data Sources

Expand Down
2 changes: 2 additions & 0 deletions GNUmakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ VERSION=$(GITTAG:v%=%)
LINKER_FLAGS=-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=${VERSION}'

GOLANGCI_VERSION=v1.55.0
MOCKERY_VERSION=v2.38.0

export PATH := $(shell go env GOPATH)/bin:$(PATH)
export SHELL := env PATH=$(PATH) /bin/bash
Expand Down Expand Up @@ -74,6 +75,7 @@ tools: ## Install dev tools
go install github.com/terraform-linters/tflint@v0.49.0
go install github.com/rhysd/actionlint/cmd/actionlint@latest
go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest
go install github.com/vektra/mockery/v2@$(MOCKERY_VERSION)
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't this pollute the go.mod file?

Copy link
Member Author

Choose a reason for hiding this comment

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

uhm, it didn' change anything in go.mod

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, double checking sometimes when doing go install and not using latest things get added to the go.mod

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin $(GOLANGCI_VERSION)

.PHONY: check
Expand Down