Skip to content

Add release scripts for the Go client #703

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

Closed
wants to merge 1 commit into from
Closed

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Apr 3, 2021

This PR introduces a couple of scripts to facilitate the release and tagging of the Go client. The motivation for this is to be able to tag versions of the client that do not include the test dependencies so that users of a tagged version of the library do not have to clone the nats-server to use the client.

As a result the go module for the Go client will look like this in tagged versions of the library:

module github.com/nats-io/nats.go

go 1.16

require (
	github.com/nats-io/nkeys v0.3.0
	github.com/nats-io/nuid v1.0.1
)

After the tagging of the repo happens, then the test dependencies are reintroduced so that go test ./.. will continue to work as usual.

Example usage

Let's say that we are releasing the v1.11.0 version of the library. First we do this:

./scripts/pre-release.sh v1.11.0

This will create a release branch called release/v1.11.0 that creates the module without test dependencies and split them into another go_test.mod which will be used instead by Travis to run CI with go test -modfile=go_test.mod ./....

After running the script, the repo with the go module without dependencies can be tagged and pushed.

Once pushing the tag and release branch, main branch is put back into development mode with all the test dependencies included:

./scripts/post-release.sh v1.11.0

Example result of this is the following app that only uses a tagged version of the client and go.sum does not include the server.

Signed-off-by: Waldemar Quevedo [email protected]
Signed-off-by: Jaime Piña [email protected]

@wallyqs wallyqs requested a review from kozlovic April 3, 2021 00:14
@coveralls
Copy link

coveralls commented Apr 3, 2021

Coverage Status

Coverage increased (+0.04%) to 86.39% when pulling 94f8211 on release-scripts into e0d6a6a on master.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Some update to .travis.yml and question about the cov.sh

.travis.yml Outdated
- go test -i -race ./...
- go test -v -run=TestNoRace -p=1 ./...
- if [[ "$TRAVIS_GO_VERSION" =~ 1.16 ]]; then ./scripts/cov.sh TRAVIS; else go test -race -v -p=1 ./... --failfast; fi
- go test -modfile=go_test.mod -i -race ./...
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we not simply remove this line. It is even deprecated since go 1.16: https://travis-ci.com/github/nats-io/nats.go/jobs/495796803#L264

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems will be fine if we just remove, so took it out: golang/go#41696

- if [[ "$TRAVIS_GO_VERSION" =~ 1.16 ]]; then ./scripts/cov.sh TRAVIS; else go test -race -v -p=1 ./... --failfast; fi
- go test -modfile=go_test.mod -i -race ./...
- go test -modfile=go_test.mod -v -run=TestNoRace -p=1 ./...
- if [[ "$TRAVIS_GO_VERSION" =~ 1.16 ]]; then ./scripts/cov.sh TRAVIS; else go test -modfile=go_test.mod -race -v -p=1 ./... --failfast; fi
Copy link
Member

Choose a reason for hiding this comment

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

Question: we use -modfile here and line above, but ./scripts/cov.sh is not. Is that a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes those entries were missing -modfile so have updated now.

Signed-off-by: Waldemar Quevedo <[email protected]>
Signed-off-by: Jaime Piña <[email protected]>
@wallyqs
Copy link
Member Author

wallyqs commented Apr 5, 2021

The minimal go.mod file will help applications be built without fetching extra dependencies, so for example an application that uses the tagged version as follows:

module github.com/wallyqs/nats-go-mod-test

go 1.16

require github.com/nats-io/nats.go v1.11.0

when the app is built using go run -mod=mod app.go then it will not fetch the test dependencies and only those listed in the go.mod that we publish will be fetched. One shortcoming right now though, is that if users runs go mod tidy, then it will fetch the test dependencies once again and put then in go.mod and go.sum. This is because go mod tidy was explicitly designed to be able to run fetch all the dependencies required to be able to run go test all in a package and run all tests including those of the tests.

In order to get around that issue, we need to refactor/move the tests that depend on the server currently in nats_test.go(https://github.com/nats-io/nats.go/blob/master/nats_test.go#L40-L41) and put them within the github.com/nats-io/nats.go/test package, so running the tests would then look like this:

# Tests that do not depend on the server.
go test github.com/nats-io/nats.go

# Tests that depend on the server
go test github.com/nats-io/nats.go/test

Once that separation is done, then nats.go users that run go mod tidy and use one of tagged released that include the minimal go.mod as suggested in the PR, then will never fetch the test dependencies.

@variadico and I took a stab at making that to see how it would like in this branch, and seems that most tests can be moved though without issues, with the exception of a couple reconnection tests such as TestReconnectServerStats and TestConnAsyncCBDeadlockt that rely on some of the private functions of the client.

@kozlovic
Copy link
Member

kozlovic commented Apr 5, 2021

@wallyqs @variadico

In order to get around that issue, we need to refactor/move the tests that depend on the server currently in..

To me this is quite annoying to have tests in different package. I like to be able to check internals. After all, we are checking the behavior of the library and sometimes it means checking that internals are what they should be. Also, in go, you should have file with _test.go for its test. Having a test package moves away from that.

I appreciate the effort, but all that we are doing here, correct me if my understanding is wrong, is to have users be able to do go test ... without the need for adding -modfile (if we were to have go.mod and go_test.mod).
My guess is that users that need nats.go library will add it to their go.mod but I don't think they will run nats's tests. If they do, then it could be asked that they use -modfile, it does not seem to be like a big ask.
Again, I appreciate the efforts you guys put into that, but is that not too much just to avoid -modfile? Or am I missing something more important?

@wallyqs
Copy link
Member Author

wallyqs commented Apr 5, 2021

Reconsidering that this is too much to effort, would be simpler to add a different modfile for the go test (#705) so closing.

@wallyqs wallyqs closed this Apr 5, 2021
@wallyqs wallyqs deleted the release-scripts branch April 5, 2021 18:58
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.

4 participants