You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/10/02 20:23:16 UTC
[GitHub] [trafficcontrol] zrhoffman opened a new pull request #5095: Update Go version to 1.15.2
zrhoffman opened a new pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
This PR
* [x] is not related to any Issue<!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
* updates the Go version from 1.14.2 to 1.15.2
* abstracts out all Go versions (other than the minimum Go version to install Goose in `traffic_ops.spec`) to get that Go version from a file at the root of the repo named `GO_VERSION`. In order to accomplish this:
- Added a Sphinx role named `:atc-go-version:` to reference the minor-level Go version we support (*1.14* before this PR) in the docs
- Replaced `golang:1.14.2-alpine` images with `alpine:3.12` and `golang:1.14.2` with `debian:buster` Dockerfiles that download the Go version specified by `GO_VERSION`. The only uses in the repo of Go in Docker where downloading Go could not be done in the Dockerfile itself were in the GitHub Actions.
- Made the Traffic Ops RPM build phase put the Go version in `install_go.sh`, instead of it being hard-coded, like we do for the ATC version in `app/lib/UI/Utils.pm`
So now, whenever we want to update Go for the whole repo, the only 2 files to change are `GO_VERSION` and `CHANGELOG.md`.
Other change:
* Makes the CDN-in-a-Box CI workflow not run if files unrelated to it in the `.github` directory are modified
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- CDN in a Box
- Documentation
- Grove
- Traffic Control Client (Go)
- Traffic Monitor
- Traffic Ops
- Traffic Ops ORT
- Traffic Stats
- CI tests
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
- Run unit tests
- Run the Traffic Ops API tests (including the system tests, which our GitHub Action does not currently cover)
- Run the various tests whose Dockerfiles were changed throughout the repo in ca43400498 and e8a77fd580
- Check the [Go 1.15 release notes](https://golang.org/doc/go1.15) to check if there's anything we are not ready for (for example, the [additional vet warnings](https://golang.org/doc/go1.15#vet) made our tests fail in 1.15 until #5084 was merged)
Note that the final status of CDN-in-a-Box CI suggests that it fails. If you open that *run* and look at which components failed, though, you should see that all jobs passed (including `ciab-build`) except for `grove` and `grovetccfg`. The reason that those 2 fail is that those are the only 2 components that actually check which Go version you are using (with `verify_and_set_go_version`) and abort the build if it's not high enough. Once the PR is merged, those jobs will pass, as well.
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR affects lots of existing tests, so running those tests should be sufficient
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#discussion_r499715993
##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -18,16 +18,26 @@
set -e
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
this one too
##########
File path: .github/actions/go-fmt/entrypoint.sh
##########
@@ -18,6 +18,16 @@
set -e
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
I know we don't mandate one or the other for shell scripts, but the rest of this file _does_ already use tabs for indentation...
##########
File path: .github/actions/to-integration-tests/entrypoint.sh
##########
@@ -16,12 +16,22 @@
# specific language governing permissions and limitations
# under the License.
-GOPATH="$(mktemp -d)"
-SRCDIR="$GOPATH/src/github.com/apache"
-mkdir -p "$SRCDIR"
-ln -s "$PWD" "$SRCDIR/trafficcontrol"
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
and this one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#issuecomment-702959349
Maybe it makes more sense to take those 2 checks out and add them back after the rest of this is merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#discussion_r499841647
##########
File path: .github/actions/go-fmt/entrypoint.sh
##########
@@ -18,6 +18,16 @@
set -e
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
Tabbed in 5f03e222b7
##########
File path: .github/actions/go-test/entrypoint.sh
##########
@@ -18,16 +18,26 @@
set -e
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
Tabs added in 5f03e222b7
##########
File path: .github/actions/to-integration-tests/entrypoint.sh
##########
@@ -16,12 +16,22 @@
# specific language governing permissions and limitations
# under the License.
-GOPATH="$(mktemp -d)"
-SRCDIR="$GOPATH/src/github.com/apache"
-mkdir -p "$SRCDIR"
-ln -s "$PWD" "$SRCDIR/trafficcontrol"
+download_go() {
+ go_version="$(cat "${GITHUB_WORKSPACE}/GO_VERSION")"
+ wget -O go.tar.gz "https://dl.google.com/go/go${go_version}.linux-amd64.tar.gz"
+ tar -C /usr/local -xzf go.tar.gz
+ rm go.tar.gz
+ export PATH="${PATH}:${GOROOT}/bin"
+ go version
Review comment:
Added tabs in 5f03e222b7
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#issuecomment-702960143
Removed check in efc6d8b3e0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#discussion_r499040058
##########
File path: docs/source/_ext/atc.py
##########
@@ -46,6 +46,29 @@ def run(self):
impl_node.append(nodes.paragraph('', '', *(n + m)))
return [impl_node]
+# -- Go Version role --------------------------------------------------
+# Returns the value of the Go version stored in GO_VERSION to minor version
+# precision.
+import os
+import re
+
+from docutils.nodes import strong
+from docutils.parsers.rst.states import Inliner
+from typing import Tuple, List
+
+def atc_go_version(unused_typ: str,
+ unused_rawtext: str,
+ unused_text: str,
+ unused_lineno: int,
+ unused_inliner: Inliner,
+ options=None,
+ content=None) -> Tuple[List[strong], list]:
+ go_version_file = os.path.join(os.path.dirname(__file__), '../../../GO_VERSION')
+ go_version = open(file=go_version_file).read()
Review comment:
Looks like this file is never closed. I'd recommend doing a context-managed read like:
```python
with open(go_version_file) as f:
go_version = f.read()
# ...
```
So that the file is always closed and fds don't leak
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#issuecomment-702960143
Removed check in c68ed42346
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#issuecomment-702958364
> Just popping in to check out the docs extension changes. Not going to look too hard at the rest of it yet, since it looks like Grove build is broken atm.
From the PR summary:
> Note that the final status of CDN-in-a-Box CI suggests that it fails. If you open that *run* and look at which components failed, though, you should see that all jobs passed (including `ciab-build`) except for `grove` and `grovetccfg`. The reason that those 2 fail is that those are the only 2 components that actually check which Go version you are using (with `verify_and_set_go_version`) and abort the build if it's not high enough. Once the PR is merged, those jobs will pass, as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#issuecomment-702960143
Removed check in 67289040da
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5095: Update Go version to 1.15.2
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5095:
URL: https://github.com/apache/trafficcontrol/pull/5095#discussion_r499050784
##########
File path: docs/source/_ext/atc.py
##########
@@ -46,6 +46,29 @@ def run(self):
impl_node.append(nodes.paragraph('', '', *(n + m)))
return [impl_node]
+# -- Go Version role --------------------------------------------------
+# Returns the value of the Go version stored in GO_VERSION to minor version
+# precision.
+import os
+import re
+
+from docutils.nodes import strong
+from docutils.parsers.rst.states import Inliner
+from typing import Tuple, List
+
+def atc_go_version(unused_typ: str,
+ unused_rawtext: str,
+ unused_text: str,
+ unused_lineno: int,
+ unused_inliner: Inliner,
+ options=None,
+ content=None) -> Tuple[List[strong], list]:
+ go_version_file = os.path.join(os.path.dirname(__file__), '../../../GO_VERSION')
+ go_version = open(file=go_version_file).read()
Review comment:
Reading with *with* in 6483fa1d46
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org