Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: Creates mock generation infrastructure #1763
chore: Creates mock generation infrastructure #1763
Changes from 23 commits
add48d9
b41ba5e
cc90022
0cbacbc
630cd1b
bb449fb
fee2a7e
c76d184
d171e9b
a3dd559
194a843
05ed39b
481fd10
9520446
92abb8f
19e3159
8907cd8
b761422
a125d08
5b7f3ad
7d1595d
4b4066b
3b478f7
5ef6390
233ce19
7191cd1
650b772
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
linter here takes 2m instead of 3m and linter version is centralized in make file
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 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!
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.
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
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.
with this change we use make lint from GH action so version is only in one place
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.
oh I see, thanks. I thought this was related somehow to the mock test but it is not.
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.
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
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.
@gssbzn that's fair but we already have a pre-commit hook with linter so linter should rarely fail in GH
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.
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.
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 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
Linter in this PR: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7259036317/job/19775411159?pr=1763
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!
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 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?
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.
why are we weakening the permissions?
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.
@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)
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.
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
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 see, thanks for the info, it looks like we'll need to a focused effort on this and change all workflows.
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.
lint done in build to centralize version in make file
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.
timeout is in config file now
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.
why did you remove lint here? if the lint fails we should not run the test
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.
the lint is now running inside build, so threre is still a dependency on it
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.
go generate syntax is deprecated in favor of mockery config file
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] 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
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. I wanted to start simple, it's very easy to change at any moment to have mocks in their resource folders.
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.
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
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.
won't this pollute the go.mod file?
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.
uhm, it didn' change anything in go.mod
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.
cool, double checking sometimes when doing go install and not using latest things get added to the go.mod
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.
curious: why this note?
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.
when using asdf, you normally need to do that when new tools are added , e.g. mockery won't work util you do that once so it's in the path. something similar happens for python for instance.
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.
ok I understand, but we never mention asdf anywhere else in the repo, right? My feeling is that this is a very targeted comment for a specific type of users, but we are not targeting those users anywhere else.
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.
we have the file .tool-versions that is specific to asdf. I'm recommending everybody to use asdf and this is something easy to forget (reshim). but you're right it's too specific, removing.
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.
removed