Additionally, gorelease should report a diagnostic if a release version is specified explicitly and that version already exists.

From #41110:

deklerk at deklerk-macbookpro in ~/workspace/lean on garbage
$ git tag
v0.1.0
deklerk at deklerk-macbookpro in ~/workspace/lean on garbage
$ gorelease -version=v0.1.0
Inferred base version: none
v0.1.0 is a valid semantic version for this release.
$ gorelease -version=v0.0.1
Inferred base version: none
v0.1.0 is a valid semantic version for this release.

As you can see, gorelease is fine with a proposed version that is equivalent to an existing version, and a proposed version that is before an existing version. This appears to be fine (tagging earlier versions is acceptable), though here I think that the base version could have been inferred. (IIRC if you pass no args at all, base version is inferred)

What other versions are available for that module?

Currently, if -version is specified, the base version is inferred to the highest version lower than -version. So if you run gorelease -version=v1.3.5, you should get a base version of v1.3.4 instead of v1.4.0, assuming both those versions exist.

Ohhhh. That makes sense. I see that above, both v0.1.0 are v0.0.1 correct. Ok. Mayyyy be worth adding a note like, "Note however that versions are before the current latest version."

That might confuse people too, and terse-ness is always nice, but yeah it is initially a bit unexpected.

Change https://golang.org/cl/288032 mentions this issue: x/cmd/gorelease: don't suggest a version that already exists

I started looking into this today. A question: we have tests like TestRelease/basic/v1_patch_suggest, which has the following setup:

mod=example.com/basic
version=v1.1.1
base=v1.1.0
-- want --
Suggested version: v1.1.1

But, there is already [...]/testadata/mod/example.com_basic_v1.1.1.txt.

So, here version v1.1.1 is being proposed but v1.1.1 already exists - seems like it should now fail (and be rewritten not to fail). (ditto for numerous similar tests)

Does that sound right?

Impl note: seems go list -json -m -versions $modpath is perfect for this. Give a shout if you think a better path is available though!

Some change to the test framework will be needed to implement this. We copy both base and release versions out of the test proxy. And in the basic module, we simulate the evolution of a module by running gorelease at several different points on its history. Those tests might run gorelease on lower existing versions, but they should act like those versions don't exist.

Just some brainstorming. These aren't necessarily good ideas:

• Each file in testdata/mod could indicate whether its version should be included in the proxy's version list (the $modpath/@v/list file).
• If a test needs to see a different version list than the proxy servers to other tests, we could set up another small directory that just has /@v/list files, only for that test, then set GOPROXY to a list containing that directory, followed by the main test proxy directory.
• gorelease could have an option that says whether we're actually releasing a version (in which case, that version should not already exist) or whether we're just comparing two versions. Tests that don't care whether a version already exists could use the second mode.

Impl note: seems go list -json -m -versions $modpath is perfect for this. Give a shout if you think a better path is available though!

Two suggestions:

• Instead of -json, use -f {{.Version}} for consistent formatting without having to unmarshal JSON.
• Eventually, we should use the -retracted flag to include retracted versions in this list. Retracted versions are omitted from this list, but retracted version numbers can't be reused, and gorelease should still warn about them. The -retracted flag is added in Go 1.16, so it won't work with older versions though. Maybe add a TODO for now.

Thanks for the thoughts, Jay!

Two suggestions:

SGTM to both.

Looked into the test restructuring. Here are some quick thoughts on the ideas presented:

Each file in testdata/mod could indicate whether its version should be included in the proxy's version list (the $modpath/@v/list file).

The proxy dir gets created at TestMain time, and tests get run in parallel, so this is a bit tricky I think.

gorelease could have an option that says whether we're actually releasing a version (in which case, that version should not already exist) or whether we're just comparing two versions. Tests that don't care whether a version already exists could use the second mode.

Figured it's a bummer to create a flag that we'd have to support, just for the sake of a test.

If a test needs to see a different version list than the proxy servers to other tests, we could set up another small directory that just has /@v/list files, only for that test, then set GOPROXY to a list containing that directory, followed by the main test proxy directory.

Exploring this idea!

Thank you again for these thoughts, it's great to have a mental foundation on which to build ideas!!

If a test needs to see a different version list than the proxy servers to other tests, we could set up another small directory that just has /@v/list files, only for that test, then set GOPROXY to a list containing that directory, followed by the main test proxy directory.

Exploring this idea!

This appears to suffer the same parallel problem as idea #1. So, I went with idea #1 and excluded the parallelism from tests with a discludeFromProxy field. See in https://go-review.googlesource.com/c/exp/+/288032: what do you think about this approach @jayconrod ?

Two things I don't love about it:

• A lot of tests need the discludeFromProxy tag, which maybe isn't so bad.
• Each test with discludeFromProxy is run in serial, which in aggregate makes tests quite a bit slower. :/

Sorry for the long reply - perf and a few fires. :)

After some thought, I'm going to just try to re-write the tests so that they don't collide with this new rule, which allows us to keep our fast concurrent tests. Ex:

$ git diff testdata/basic/v0_compatible_suggest.test
diff --git a/cmd/gorelease/testdata/basic/v0_compatible_suggest.test b/cmd/gorelease/testdata/basic/v0_compatible_suggest.test
index a3be718..82ed794 100644
--- a/cmd/gorelease/testdata/basic/v0_compatible_suggest.test
+++ b/cmd/gorelease/testdata/basic/v0_compatible_suggest.test
@@ -1,7 +1,5 @@
 mod=example.com/basic
-version=v0.1.0
-base=v0.0.1
-discludeFromProxy=v0.1.0
+base=v0.1.2
 -- want --
 example.com/basic/a
 -------------------
@@ -13,4 +11,17 @@ example.com/basic/b
 Compatible changes:
 - package added

-Suggested version: v0.1.0
+Suggested version: v0.2.0
+-- go.mod --
+module example.com/basic
+
+go 1.12
+-- a/a.go --
+package a
+
+func A() int { return 0 }
+func A2() int { return 2 }
+-- b/b.go --
+package b
+
+func B() int { return 3 }

If we can keep the existing tests (mostly) the same, that would be better. Here's what I'd suggest:

• Add a proxyversions setting for tests that need it. It should accept a comma-separated list of versions that our proxy will say is available. If it's not specified, we don't do anything special, and all versions will be available (as they are now).
• When setting up a test, if proxyversions is set, we'll create two proxy directories.
  • The first directory will contain files like $mod/@v/list where $mod is the module path. The list file should contain a newline-separated list of available versions. (Docs at https://golang.org/ref/mod#goproxy-protocol; see https://proxy.golang.org/golang.org/x/mod/@v/list for an example). This may be different for each test.
  • The second directory is the one we create now. It will be the same for all tests.
• We'll need to set GOPROXY to a list of either one or two file:// URLs for the two directories, depending on whether proxyversions is set.
  • Unfortunately, we can't set that globally and run tests in parallel at the same time. Maybe we can thread a list of environment variables through the runRelease function down to all the places where we invoke the go command? Then we can set the environment variables for each command in exec.Cmd.Env instead of setting them globally.
  • One way to do this would be to pass a context.Context to runRelease and any other functions that need it. We could store the environment list using context.WithValue. What do you think? We should probably be using Context anyway to make cancellation with ^C a little smoother.

These are great ideas. Starting work today on trying this pipe-env-list approach.

Done: https://go-review.googlesource.com/c/exp/+/288032

Sorry it took so long.