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

x/exp/cmd/gorelease: don't suggest a version that already exists #37562

Closed
jayconrod opened this issue Feb 28, 2020 · 14 comments
Closed

x/exp/cmd/gorelease: don't suggest a version that already exists #37562

jayconrod opened this issue Feb 28, 2020 · 14 comments

Comments

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 28, 2020

When gorelease is used without the -version flag, it may suggest a version automatically, derived from the -base version.

If the version that gorelease would normally recommend already exists, gorelease should note this.

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Aug 31, 2020

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

@jadekler
Copy link
Member

@jadekler jadekler commented Aug 31, 2020

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)

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Aug 31, 2020

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.

@jadekler
Copy link
Member

@jadekler jadekler commented Aug 31, 2020

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.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 29, 2021

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

@jadekler
Copy link
Member

@jadekler jadekler commented Jan 29, 2021

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!

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Jan 29, 2021

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.
@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Jan 29, 2021

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.
@jadekler
Copy link
Member

@jadekler jadekler commented Feb 7, 2021

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!!

@jadekler
Copy link
Member

@jadekler jadekler commented Feb 7, 2021

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. :/
@jadekler
Copy link
Member

@jadekler jadekler commented Feb 26, 2021

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 }
@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Feb 26, 2021

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.
  • 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.
@jadekler
Copy link
Member

@jadekler jadekler commented Mar 12, 2021

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

@jadekler
Copy link
Member

@jadekler jadekler commented Apr 2, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants