search

fabric8-services / fabric8-wit

wit stands for Work Item Tracker
http://devdoc.almighty.io/
Apache License 2.0
45 stars 86 forks source link

Remove USE_GO_VERSION_FROM_WEBSITE flag from dockerfile #2376

Closed jarifibrahim closed 5 years ago

jarifibrahim commented 5 years ago

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

codecov[bot] commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@            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 commented 5 years ago

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

jarifibrahim commented 5 years ago

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.

alexeykazakov commented 5 years ago

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 commented 5 years ago

@alexeykazakov @kwk some background: https://github.com/openshiftio/openshift.io/issues/4631#issuecomment-446450573

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

jarifibrahim commented 5 years ago

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

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 commented 5 years ago

[test]

alexeykazakov commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/fabric8-services/fabric8-cluster/pull/47 for example as an option for fixing that.

kwk commented 5 years ago

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 commented 5 years ago

Closing this PR in favour of https://github.com/fabric8-services/fabric8-wit/pull/2378.