Remove USE_GO_VERSION_FROM_WEBSITE flag from dockerfile

[jarifibrahim]

This PR removes the USE_GO_VERSION_FROM_WEBSITE flag from the dockerfile. We don't need two different ways of installing golang.

[codecov]

Codecov Report

Merging #2376 into master will decrease coverage by 2.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2376      +/-   ##
==========================================
- Coverage   69.84%   67.57%   -2.27%     
==========================================
  Files         171      182      +11     
  Lines       17054    17859     +805     
==========================================
+ Hits        11911    12069     +158     
- Misses       3972     4615     +643     
- Partials     1171     1175       +4

Impacted Files	Coverage Δ
goasupport/conditional_request/generator.go	20.27% <0%> (ø)
gormsupport/cleaner/db_clean.go	100% <0%> (ø)
resource/require.go	78.26% <0%> (ø)
goasupport/jsonapi_errors_stringer/generator.go	0% <0%> (ø)
main.go	0% <0%> (ø)
goamiddleware/jwt_token_context.go	0% <0%> (ø)
goamiddleware/jwt_shared.go	0% <0%> (ø)
gormapplication/application.go	70.66% <0%> (ø)
goasupport/helper_function/generator.go	0% <0%> (ø)
tool/wit-cli/main.go	0% <0%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce00568...5c3285b. Read the comment docs.

[alexeykazakov]

We should not use go from website in our builds. It's OK to use it for tests though.

[jarifibrahim]

We should not use go from website in our builds. It's OK to use it for tests though.

Yes agreed. We will fix that separately. The intent of this PR is to get rid of the flag in dockerfile.

[kwk]

We don't need two different ways of installing golang.

Just as an explanation for why we have that flag: We wanted to have a way to use the go test cache which wasn't available at that time from the version that centos provided. That is why I've added the flag so we can make use of a newer golang version when we run our coverage builds. That was the only place in which we've used it.

@jarifibrahim please confirm that we have a version of Go available in CentOS that supports the go test cache. Otherwise our coverage builds will be much slower if we remove that flag now.

See this PR for more information about the time improvements:

#1904

Apart from the above concern for why we added the flag initially, I would like to keep it because we never know what bits will be there that need testing only in a special case, that's why the flag was explicitly kept so generic. Just because we might no longer need it now, I would still like to keep the flag so we can have a smooth transitions to Go modules (@sbose78 @alexeykazakov @xcoulon ).

As far as I know, this flag doesn't hurt anybody, does it?

[alexeykazakov]

Apart from the above concern for why we added the flag initially, I would like to keep it because we never know what bits will be there that need testing only in a special case, that's why the flag was explicitly kept so generic.

@kwk +1
This is exactly why we still keep this flag in our Platform/core repos even if they are not currently used. We might want to use them later to test different versions of go.

[sbose78]

@alexeykazakov @kwk some background: openshiftio/openshift.io#4631 (comment)

Btw, we haven't decided on removing this yet - we are experimenting.

[jarifibrahim]

@kwk @alexeykazakov We decided (see https://chat.openshift.io/developers/pl/d9m8pheuhpywzpyo5qd91nuf7h) to remove the flag because we have two builds

• https://ci.centos.org/view/Devtools/job/devtools-fabric8-wit-build-master/612/consoleFull and
• https://ci.centos.org/view/Devtools/job/devtools-fabric8-wit-build-master/614/consoleFull

both of them are using different versions of golang.

Interestingly, if you look at all the builds before build 612, we were installing golang 1.10 (via the website) but golang 1.9.4 was being used to build/test (Search for INFO: go in any build before 614)

@jarifibrahim please confirm that we have a version of Go available in CentOS that supports the go test cache. Otherwise our coverage builds will be much slower if we remove that flag now.

@kwk This PR changes nothing. We have been installing golang from the website all this time and after the flag is removed we will still be installing golang from the website. I have only removed the flag. Everything else remains the same.

Apart from the above concern for why we added the flag initially, I would like to keep it because we never know what bits will be there that need testing only in a special case, that's why the flag was explicitly kept so generic. Just because we might no longer need it now, I would still like to keep the flag so we can have a smooth transitions to Go modules

@kwk the flag isn't adding any value right now. Removing it would mean we don't have any confusion about where/what version of golang is being installed. If we need to use a different version of golang, we could change it. It won't be that

[jarifibrahim]

[test]

[alexeykazakov]

Interestingly, if you look at all the builds before build 612, we were installing golang 1.10 (via the website) but golang 1.9.4 was being used to build/test (Search for INFO: go in any build before 614)

Yes, you always install golang 1.10 from website (due to bug in docke builder file) but when golang was available in centos (it was when 612 was built) it was installed first and you ended up having two go binaries in PATH. But go 1.9 was first so it was used.

By the time 614 was built golang package had been removed from centos. So, only go from website (1.10) was installed. This is why you see go 1.10 in 614.

[jarifibrahim]

Yes, you always install golang 1.10 from website (due to bug in docke builder file) but when golang was available in centos (it was when 612 was built) it was installed first and you ended up having two go binaries in PATH. But go 1.9 was first so it was used.

So the installation of both, go 1.10 and go 1.9 in the builder container was intentional? I thought we were installing only one of those.

[alexeykazakov]

So the installation of both, go 1.10 and go 1.9 in the builder container was intentional? I thought we were installing only one of those.

No, it was not intentional. It's a bug. Incorrect usage of test -z/n in Dockerfile.builder
test -z/n just checks if the env var is an empty string or not. So setting it to 0 or 1 is the same thing. It will always trigger installation from website.

Look at fabric8-services/fabric8-cluster#47 for example as an option for fixing that.

[kwk]

Look at fabric8-services/fabric8-cluster#47 for example as an option for fixing that.

@alexeykazakov thanks for pointing to that PR. @jarifibrahim can you make a fix according to that one instead?

I object to remove the flag entirely!

[jarifibrahim]

Closing this PR in favour of #2378.