Change go version detection logic

[soltysh]

Since some of us are already using go 1.7 we're missing certain verification mechanism out of the box. I've changed the logic to check only minimal go version. @stevekuznetsov ptal this part

Unfortunately, govet still complains about:

$ hack/verify-govet.sh 
pkg/auth/ldaputil/client.go:69: assignment copies lock value to c: crypto/tls.Config contains sync.Once contains sync.Mutex
pkg/cmd/util/net.go:126: assignment copies lock value to transport: net/http.Transport contains sync.Mutex

@deads2k @liggitt any ideas how to fix those?

[stevekuznetsov]

we want to use the following func formatting:

function name() {

}
readonly -f name

[stevekuznetsov]

To explain: the formatting is for consistency, the readonly is for scripts that call other scripts where both source hack/lib/init.sh

[liggitt]

@deads2k @liggitt any ideas how to fix those?

for the first, not really... technically copying the config is wrong because the new one shares a map of session tickets, guarded by a copy of a mutex, but that's only used by servers, and this is for a client. also, go makes the same copy.

for the second, we should be using net.SetTransportDefaults rather than making a copy of the default client's transport

[soltysh]

With PERMISSIVE_GO these govet errors won't show-up in travis, but I'll open separate issues to address them.

@stevekuznetsov comments addressed, ptal once again.

[stevekuznetsov]

This was my fault in the previous comment, but these should be os::log::error since they mean something failed.

[soltysh]

@stevekuznetsov updated

[stevekuznetsov]

I would make this look like:

if [[ "${go_version[2]}" != "go1.6" ]]; then
    os::log::info "Detected go version: ${go_version[*]}."
    if [[ -z "${PERMISSIVE_GO:-}" ]]; then
        os::log::error "Please install Go version 1.6 or use PERMISSIVE_GO=y to bypass this check."
        return 1
    else
        os::log::warn "Detected golang version doesn't match preferred Go version for Origin."
        os::log::warn "This version mismatch could lead to differences in execution between this run and the Origin CI systems."
        return 0
    fi
fi

[soltysh]

@stevekuznetsov comments addressed.

[stevekuznetsov]

Bash LGTM

[soltysh]

===== Verifying UPSTREAM Commits =====
SUCCESS: All commits are valid.
hack/verify-gofmt.sh
[INFO] Detected go version: go version devel +111d590 Fri Jul 29 01:09:55 2016 +0000 linux/amd64.
[ERROR] Please install Go version 1.6 or use PERMISSIVE_GO=y to bypass this check.

@stevekuznetsov what's your plan to deal with the tip version of go we use in travis?

[stevekuznetsov]

I'd have to ask @smarterclayton what, exactly, we gain by running a tip build on Travis first. We could just pass the permissive flag in to Travis.

[smarterclayton]

We do it to flag early go build problems coming from upstream.

[stevekuznetsov]

Ok, then letting it be permissive should be fine, we can catch updates for
go tool vet as well.

On Jul 30, 2016 6:30 PM, "Clayton Coleman" [email protected] wrote:

We do it to flag early go build problems coming from upstream.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10101 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AG_ScjvRvfbNm0jbu_ylRbJ_rCpCCLmBks5qa9BmgaJpZM4JYBXs
.

[soltysh]

@stevekuznetsov for travis part and pkg/auth/ldaputil/client.go since you touched it
@liggitt for the remaining go code

[soltysh]

[test]

[stevekuznetsov]

re[test]

[soltysh]

@liggitt, Steve suggested we go with ignoring certain errors and that's the final solution. @stevekuznetsov ptal

[stevekuznetsov]

@liggitt, Steve suggested we go with ignoring certain errors and that's the final solution

To expand on this, the reason was that there is no sane way to have a Copy() method for the TLSConfig that satisfied the 1.6 and 1.7 golang stdlibs, so we will continue to do the copy unsafely and just follow up by placing a new lock into the copy manually after the fact (@soltysh that's still a TODO)

[stevekuznetsov]

@soltysh we don't want to remove this part of hack/verify-govet.sh

[soltysh]

Why, I haven't see us using it?

[soltysh]

@stevekuznetsov updated except for the removed chunk, I don't see why we need it if it's of no use, unless I'm missing something?

[soltysh]

ptal

[soltysh]

Reverted back the block @stevekuznetsov was asking for and added a two more ignores to the generated clientset, which will require a bit more of work to have them pass govet.

[stevekuznetsov]

LGTM on a green [test] -- maybe we should open issues for those lock copies that are our fault

[openshift-bot]

Evaluated for origin test up to 0ffa6e6

[soltysh]

Issue created #10357.

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7790/)

[soltysh]

Tests are green.

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7790/) (Image: devenv-rhel7_4806)

[openshift-bot]

Evaluated for origin merge up to 0ffa6e6