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

doc: BuildNameToCertificate deprecated in go 1.14 not mentioned in the release notes #37626

Open
coderste opened this issue Mar 3, 2020 · 11 comments

Comments

@coderste
Copy link

@coderste coderste commented Mar 3, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/shi03/Library/Caches/go-build"
GOENV="/Users/shi03/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/shi03/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/shi03/code/apis/gs/capi/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/dc/pxx2c5t503jdfy7c29qrlmp9nkvqm9/T/go-build781481956=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Looks like the BuildNameToCertificate function was deprecated in 1.14 but there's no mention of this on the release page.

What did you expect to see?

Release page mentioning the deprecated

What did you see instead?

No mention of the deprecation

@coderste coderste closed this Mar 3, 2020
@coderste coderste reopened this Mar 3, 2020
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 3, 2020

The release notes are meant to provide a high level description of changes. Every single change that went into Go 1.14 cannot be included. I'm not sure if this is within scope of release notes, since it's easy to see that a function is deprecated by looking at package documentation.

I'll let package owners decide if anything should be done.

/cc @FiloSottile @katiehockman @ianlancetaylor

@dmitshur dmitshur changed the title BuildNameToCertificate deprecated in go 1.14 not mentioned in the release notes doc: BuildNameToCertificate deprecated in go 1.14 not mentioned in the release notes Mar 3, 2020
@dmitshur dmitshur added this to the Go1.15 milestone Mar 3, 2020
@coderste

This comment has been minimized.

Copy link
Author

@coderste coderste commented Mar 4, 2020

Ah fair enough I just thought it was missed rather than being done intentionally. In that case happy for this to be closed as resolved :)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 4, 2020

I certainly agree that not every change can be mentioned in the release notes, but I do tend to feel that all new functions should be documented and, correspondingly, that all decisions to deprecate a function should be documented.

@Sherlock-Holo

This comment has been minimized.

Copy link

@Sherlock-Holo Sherlock-Holo commented Mar 23, 2020

make something deprecated is not common, a lot of people only read release notes to consider updating go version or not, then make a test. We invoke a lot of functions and if we need to read every function document to see if it is deprecated or not, it is a bad idea.

By the way, even I still use BuildNameToCertificate, go vet doesn't report it. If the Goland doesn't tell me this function is deprecated, maybe I use it even update to go 1.15

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 23, 2020

Happy to add it to the release notes.

@Sherlock-Holo When something is deprecated it does not stop working, and go vet does not fail due to use of deprecated identifiers, that's expected. Other static analysis tools like staticcheck do.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 23, 2020

@dmitshur this will need backporting to show up on golang.org right?

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 23, 2020

@FiloSottile Yes, to release-branch.go1.14, since the file to be modified is in the main Go repository.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 23, 2020

@gopherbot please open a backport issue for Go 1.14, this is a documentation change that needs to go live on golang.org.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 23, 2020

Backport issue(s) opened: #38030 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@Sherlock-Holo

This comment has been minimized.

Copy link

@Sherlock-Holo Sherlock-Holo commented Mar 23, 2020

@FiloSottile when something is deprecated, it is not a good idea for continuing using it, if it is not defective, why we deprecate it? IMO, the better idea is finding another way to replace it.

use tool likes staticcheck is a good idea, but writing it into release notes can let people know it is deprecated as soon as possible

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 23, 2020

There are a few reasons to deprecate something. In this case NameToCertificate is limited in its flexibility and fully replaced by better Certificates selection logic, so no new application should use it. Hence, deprecation. However, if an application is currently using it and it works for them, there is no urgency to switch away from it. We don't break applications unless strictly necessary.

I don't disagree about highlighting it in the release notes, though!

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
6 participants
You can’t perform that action at this time.