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 module path to have the major version to comply with go modules specification. #10640

Merged
merged 1 commit into from May 2, 2019

Conversation

shrajfr12
Copy link
Contributor

@shrajfr12 shrajfr12 commented Apr 12, 2019

The module path in go.mod is go.etcd.io/etcd. Current version tag is v3.3.12.

This does not comply with go module specification as explained in in https://github.com/golang/go/wiki/Modules#semantic-import-versioning. (Refer to the bulleted item starting with "If the module is version v2 or higher")
To comply with this rule, the module path needs to have /v3 at the end in go.mod file.

This PR brings the module path in compliance with go modules specs. The main change in this PR is changing the go.mod to refer to the module as go.etcd.io/etcd/v3.
The huge list of files changed is simply changing the imports to now refer to the etcd module with the new name having /v3 at the end.
It is assumed that this will be tagged with the next higher patch or minor version (e.g v3.3.13 or v3.4.0 )
I have also changed the travis.yml to set GO111MODULE to "on" so that the go module functionality is activated and passed this env variable to the docker run command as well. . (With upcoming go1.13, this will be the default) to ensure go modules functionality is always enabled.
I have run unit, functional, e2etests and integration tests. With the fmt and bom tests, some tools are not yet go modules compatible and so they are excluded(relevant issues documented in script "test")
Some more cleanups (like removing vendor directories and fixing some of the scripts can be taken up later in new PRs.) Please note that we will have to do this process all over again when we change major version in tags to v4 and v5 and so on in order to be compliant with go modules specs, but I made this PR since I saw in ROADMAP document that v3.4 the intention of etcd team is to adopt go modules fully.

@shrajfr12 shrajfr12 changed the title Fix module path to have the major version to comply with go modules specification. [wip] Fix module path to have the major version to comply with go modules specification. Apr 12, 2019
@shrajfr12 shrajfr12 changed the title [wip] Fix module path to have the major version to comply with go modules specification. [WIP] Fix module path to have the major version to comply with go modules specification. Apr 12, 2019
@shrajfr12 shrajfr12 force-pushed the gomodulecompat branch 2 times, most recently from 0bcf512 to c3a7df7 Compare April 26, 2019 21:39
@shrajfr12 shrajfr12 changed the title [WIP] Fix module path to have the major version to comply with go modules specification. Fix module path to have the major version to comply with go modules specification. Apr 26, 2019
@xiang90
Copy link
Contributor

xiang90 commented May 1, 2019

/cc @gyuho

@gyuho gyuho merged commit e899023 into etcd-io:master May 2, 2019
@jpbetz jpbetz mentioned this pull request May 14, 2019
mitake added a commit to mitake/etcd that referenced this pull request May 17, 2019
It seems that 9150bf5 of etcd-io#10640
broke import paths of proto files. Because of this scripts/genproto.sh
causes errors on the latest master branch. This commit fixes the
broken paths.
@gyuho
Copy link
Contributor

gyuho commented May 28, 2019

I am reverting this, until we are finding resolutions for proto import path issues.

If the module is version v2 or higher, the major version of the module must be included as a /vN at the end of the module paths used in go.mod files (e.g., module github.com/my/mod/v2, require github.com/my/mod/v2 v2.0.0) and in the package import path (e.g., import "github.com/my/mod/v2/mypkg").

I understand this as having a versioned directory. Currently, we do not want to maintain two major versioned implementation in separate directories.

I do not see any benefit of doing this for now.

@shrajfr12
Copy link
Contributor Author

Hi @gyuho ,I am not sure I understand the concern. There is no need for a version directory to exist. Module path name should include the version in the path and the imports should specify the path. When go modules are enabled go will import based on the module version in the path.

@gyuho
Copy link
Contributor

gyuho commented May 28, 2019

See https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher.

Major subdirectory: Create a new v3 subdirectory (e.g., my/module/v3) and place a new go.mod file in that subdirectory. The module path must end with /v3. Copy or move the code into the v3 subdirectory. Update import statements within the module to also use /v3 (e.g., import "github.com/my/module/v3/mypkg"). Tag the release with v3.0.0.

Plus, tools around etcd's dependency do not work well with this change.

We will come back to this, after we fix all the tests.

Can you create a separate issue?

@shrajfr12
Copy link
Contributor Author

The subdirectory approach you referenced is Option 2. Instead Option 1 was used in the PR to avoid multiple directory creation with each version and this approach works fine when converting to go modules. Either approach can be used. But we can definitely revisit this later so that the issues with tools/scripts around etcd can be addressed. If there are sync-up meetings scheduled, please do let me know so I can join and understand more details about the issues with tools in etcd project when go modules are enabled. Thanks

@shrajfr12
Copy link
Contributor Author

@gyuho , On further review, I found that the script "genproto.sh" is explicitly disabling GO111MODULE to off, so this is not go module aware. But this script does not seem to be getting called during PR builds, so any reason for that ? If it is not needed in the build, what are the likely use cases for executing this script manually ? And are there any additional scripts that fall in the same category ? Once the answers to these questions are clear, I will open a new issue as you suggested. Thanks

@ankushchadha
Copy link

Hi @gyuho - can we get more details about genproto.sh? We would like to make this script to be module aware so that this PR can be merged to make the etcd project module aware.

@havardk havardk mentioned this pull request Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants