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

Bump recommended Go development version in README #1340

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

znewman01
Copy link
Contributor

Summary

#568 bumps the Go version for tests to 1.17. Now they break for 1.16:

$ TMPDIR=$(mktemp -d)
$ cd $TMPDIR
$ git clone https://github.com/sigstore/cosign 2> /dev/null
$ cd cosign
$ for go in go_1_16 go_1_17; do
>   nix-shell \
>     -p ${go} \
>     -p gnumake \
>     --command 'make test' \
>     > /dev/null 2>&1
>   if [ $? -eq 0 ]; then
>     echo ${go} good
>   else
>     echo ${go} bad
>   fi
> done
go_1_16 bad
go_1_17 good

#1252 adds a call to testing.T.Setenv, added in 1.17, which caused this breakage.

(We're still testing builds on 1.16, so that's fine.)

Ticket Link

Fixes N/A

Release Note

NONE

CI only runs tests on 1.17 (it still runs *builds* on 1.16, so the other
references in the README are fine). Further, builds *are* broken on
1.16 right now.

Signed-off-by: Zachary Newman <z@znewman.net>
@@ -30,7 +30,7 @@ For Linux and macOS binaries see the [GitHub release assets](https://github.com/

## Developer Installation

If you have Go 1.16+, you can setup a development environment:
If you have Go 1.17+, you can setup a development environment:
Copy link
Member

Choose a reason for hiding this comment

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

if we bump here in the readme, we should change the go module as well

cc @mattmoor @dlorenc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not an expert on Go modules but AFAICT the Go version in go.mod has 2 effects:

  1. Restricts language features introduced after that version.
  2. Changes the behavior of the go command. In particular, for the 1.16->1.17 transition the consequential changes:
    • starts including explicit requires in go.mod for transitive dependencies
    • vendoring acts a little different (starts recording dependencies' Go versions from go.mod, omits go.mod and go.sum for vendored dependencies)

Because as of right now we still want to support 1.16, my initial thought is that (1) is a good thing for development and I'm inclined not to change it. Of course, will defer to you all :)

Copy link
Member

Choose a reason for hiding this comment

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

We chatted about bumping the other one to 1.17 soon, this one seems fine as is and we can do the other one later.

@cpanato cpanato requested a review from dlorenc January 19, 2022 15:31
@@ -30,7 +30,7 @@ For Linux and macOS binaries see the [GitHub release assets](https://github.com/

## Developer Installation

If you have Go 1.16+, you can setup a development environment:
If you have Go 1.17+, you can setup a development environment:
Copy link
Member

Choose a reason for hiding this comment

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

We chatted about bumping the other one to 1.17 soon, this one seems fine as is and we can do the other one later.

@dlorenc dlorenc merged commit 1a92b50 into sigstore:main Jan 19, 2022
@github-actions github-actions bot added this to the v1.5.0 milestone Jan 19, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
CI only runs tests on 1.17 (it still runs *builds* on 1.16, so the other
references in the README are fine). Further, builds *are* broken on
1.16 right now.

Signed-off-by: Zachary Newman <z@znewman.net>
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.

3 participants