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

[MODULES] Switch to Go Modules #356

Closed
wants to merge 4 commits into from
Closed

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Mar 14, 2019

Closes #293

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Looks like we'll need to investigate that one test failure 🤔

=== RUN   TestAccIdentityGroupExternal
--- FAIL: TestAccIdentityGroupExternal (0.09s)
    testing.go:519: Step 1, no error received, but expected a match to:
        
        cannot set 'member_entity_ids' on external groups

@appilon
Copy link
Contributor Author

appilon commented Mar 18, 2019

@radeksimko yes this was peculiar, only hunch I had was this attribute uses a diffsuppressfunc and we upgraded from tf 0.10 to tf 0.11

@cvbarros
Copy link
Contributor

cvbarros commented Mar 18, 2019

I can't even understand how this test would pass, the code on master doesn't throw that error anymore. Got removed by 909c5bd.
Maybe the upgrade from 0.10 to 0.11 is now causing a diff. I think to get around that maybe needs to use CustomizeDiff on the entire resource?

Edit: I think that now it's safe to remove the test since 909c5bd

@dhohengassner
Copy link

would like to see this PR merged soon - nice job!

@radeksimko
Copy link
Member

@terraform-providers/vault Could you please confirm it is safe to remove that test and/or raise a separate PR to remove it?

@jefferai
Copy link
Member

Can this wait until Vault switches?

appilon added 4 commits March 20, 2019 11:10
provider: Run go fix
provider: Run go fmt
provider: Encode go version 1.11.5 to .go-version file
run go mod tidy
remove govendor from makefile and travis config
set appropriate env vars for go modules
had to update transitive cloud.google.com/go to fix errors
@appilon appilon force-pushed the go-modules-2019-03-14 branch from a9db8ca to 1626014 Compare March 20, 2019 15:10
@appilon
Copy link
Contributor Author

appilon commented Mar 20, 2019

The tests pass now!

@appilon appilon requested a review from a team March 20, 2019 15:18
@cvbarros cvbarros mentioned this pull request Mar 21, 2019
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Copying what I sent on Slack:

The switch wasn't done with Go 1.12 although we've found lots of bugs with gomod in Go 1.11. The PR is so big that even on my beefy desktop I can't get it to scroll without huge delays, which makes it pretty impossible to review, so reducing scope would be nice. Also this combines a lot of code changes with the gomodules switch which makes it hard to understand what's going on.

I think this PR needs to be split into separate steps. One: update existing deps with govendor and fix code breakages. Two: Switch to go mod. Three: Update vendor folder.

@appilon
Copy link
Contributor Author

appilon commented Mar 29, 2019

I will close this PR and open one just bumping the tf sdk

@appilon appilon closed this Mar 29, 2019
@paultyng paultyng deleted the go-modules-2019-03-14 branch June 19, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants