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

docs: add ADR 053 Go Module Refactoring #11799

Merged
merged 5 commits into from
Apr 28, 2022

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Apr 27, 2022

Description

This adds an ADR to describe our process of refactoring the SDK into standalone go modules as discussed in #10582 and started in #10779 and #11788.

rendered


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Apr 27, 2022
@aaronc aaronc requested a review from fdymylja April 27, 2022 16:08
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Great and simple write-up.

@alexanderbez
Copy link
Contributor

@marbar3778 where are we with the domain name for the repos?

@alexanderbez alexanderbez merged commit 597ab54 into main Apr 28, 2022
@alexanderbez alexanderbez deleted the aaronc/go-module-refactoring-adr branch April 28, 2022 12:52
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Nice write-up! Apart from semver, the biggest win here for me is shipping speed.

Should we leave ADRs open for a longer time? e.g. at least until the next architecture call. This could do with inputs from exterior teams

discussed below in the [Consequences](#consequences) section), but the
approach being adopted is the following:
* a go module should generally be scoped to a specific coherent set of
functionality (such as math, errors, store, etc.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth mentioning that each SDK module will be its own go module too?

being tagged as `v1.0.0` to accommodate the possibility that they may be
better served by a standalone repository in the future
* all go modules should follow the guidelines in https://go.dev/blog/module-compatibility
before `v1.0.0` is tagged and should make use of `internal` packages to limit
Copy link
Contributor

Choose a reason for hiding this comment

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

For x/* go modules, there some probably more guidelines, right? Some unanswered questions I have, which I think could go into this ADR:

  • do state-machine breaking changes require a major bump?
  • client-breaking changes?
  • CLI?


* there will be more go module versions to update in the SDK itself and
per-project, although most of these will hopefully be indirect

Copy link
Contributor

Choose a reason for hiding this comment

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

Another negative one is that I think we need tooling for publishing go modules. Similar to lerna for go modules.

Say foo@1.0.0 has a security fix, so we release foo@1.0.1. And we want a new patch version for each of {bar,baz,qux} go modules. Currently we would need to update deps manually (or using dependabot), and then tag these individually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a Tooling subsection would actually make sense, but can be left empty for this version of the ADR, since this ADR is still only proposed.

Comment on lines +73 to +74
In general, it seems safer to just create a new module path (appending v2, v3, etc.
if necessary), rather than trying to make an old package a new module.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is a bit confusing to me. It seems for errors, we did create a new module from an old package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing that's not clear for me is: when bumping a major version, do we:

  • rename in go.mod cosmossdk.io/foo -> cosmossdk.io/foo/v2, and tag foo/v2.0.0
  • or no rename in go.mod, simply tag foo/v2.0.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that go.mod module directive has to be updated, but moving code to subdirectory is optional:

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. For example, the module with path golang.org/x/repo/sub/v2 could be in the /sub or /sub/v2 subdirectory of the repository golang.org/x/repo.

ref: https://go.dev/ref/mod

* standalone pieces of software will reach `v1.0.0` sooner
* new features to specific functionality will be released sooner

### Negative
Copy link
Contributor

@amaury1093 amaury1093 Apr 28, 2022

Choose a reason for hiding this comment

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

Currently SDK modules depend on other modules' public APIs. With independent go module versioning, we might need to have compatibility tables like staking@v5 is compatible with bank@{v3,v4} but not with bank@v2 etc, is that correct?

It seems this can be elegantly solved by ADR-033 and the api/ folder (no go API compatibility, on proto comptability), but until then, is that a concern to you too @aaronc ?

@alexanderbez
Copy link
Contributor

Sorry @AmauryM the PR was merged prior to your input, which I think is super valuable. @aaronc do you think these comments are worth addressing in a quick follow up PR?

@aaronc
Copy link
Member Author

aaronc commented Apr 28, 2022

Sorry @AmauryM the PR was merged prior to your input, which I think is super valuable. @aaronc do you think these comments are worth addressing in a quick follow up PR?

Yes I will make a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants