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

Fix go module path #961

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Conversation

turkenh
Copy link
Contributor

@turkenh turkenh commented Oct 30, 2021

Description of the change

This PR fixes go module path per https://golang.org/ref/mod#module-path:

If the module is released at major version 2 or higher, the module path must end with a major version suffix like /v2. This may or may not be part of the subdirectory name.

Appends /v4 since the current major is v4.

Also fixes the repository path which seems to be legacy after migrating from https://github.com/hashicorp/terraform-provider-github.

How has this code been tested

  1. Run make build
  2. Verify the code compiles and builds the plugin binary

Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh changed the title Fix go module path per https://golang.org/ref/mod#module-path Fix go module path Oct 30, 2021
@jcudit jcudit added this to the v4.19.0 milestone Nov 2, 2021
@jcudit
Copy link
Contributor

jcudit commented Nov 2, 2021

After reviewing this we agree this is the right move from a Go project standpoint and would like to move forward with the changes.

Hesitating here a bit to consider backwards compatibility and will return with more details. In the meantime, can you provide some more information around what obstacle this creates for your use case?

@turkenh
Copy link
Contributor Author

turkenh commented Nov 3, 2021

Thanks for reviewing this @jcudit.

In Terrajet project, we are working on generating Crossplane providers by leveraging existing Terraform providers. We are importing the provider package for its schema and this issue prevents us from importing the latest version. We are aware that it is not the suggested way and planning to change though.

Given that this is unsupported, I couldn't insist on merging this. However, apart from our use case, I believe it is the correct way and could just be merged if it doesn't break anything :)

@jcudit jcudit modified the milestones: v4.19.0, v4.18.1, v4.18.2 Nov 15, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

🤞🏾 this works as intended. Will keep an eye out for use cases we haven't considered that this may break 👀

@jcudit
Copy link
Contributor

jcudit commented Nov 30, 2021

I'm having trouble running a basic test locally with this branch as-is:

$ TF_ACC=1 go test -v -count=1 -timeout 30m -run ^TestAccGithubBranchProtection/configures_default_settings_when_empty$ github.com/integrations/terraform-provider-github/github
cannot find package "." in:
        /Users/jcudit/integrations/terraform-provider-github/vendor/github.com/terraform-providers/terraform-provider-github/github

May not be as straightforward as I'd hoped. Aiming to revisit as part of the v5 release.

@jcudit jcudit modified the milestones: v4.18.2, v5.0.0 Nov 30, 2021
@kfcampbell kfcampbell modified the milestones: v5.0.0, v4.24.0 Mar 25, 2022
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

I'm unable to reproduce the errors @jcudit was seeing earlier; tests run for me locally with these changes. In addition, I've asked my coworker Alan about the safety of doing this, and I was reassured that it's okay.

I'll merge this shortly!

@kfcampbell kfcampbell merged commit fdfd3c6 into integrations:main Apr 8, 2022
kfcampbell added a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…ations#961)

Signed-off-by: Hasan Turken <turkenh@gmail.com>

Co-authored-by: Keegan Campbell <kfcampbell@github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
kazaker pushed a commit to auto1-oss/terraform-provider-github that referenced this pull request Dec 28, 2022
…ations#961)

Signed-off-by: Hasan Turken <turkenh@gmail.com>

Co-authored-by: Keegan Campbell <kfcampbell@github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
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.

3 participants