-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
test: ensure that build URLs are valid #2289
Conversation
Summary: This tests that URLs point to the TensorFlow mirror (as opposed to the Bazel mirror) and that they resolve to valid archives. Test Plan: Run `./tensorboard/tools/mirror_urls_test.sh`, and note that it passes with no output. Then apply the following patch to add some bad URLs: ```diff diff --git a/WORKSPACE b/WORKSPACE index 3943b0cf..d3975dfe 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -9,6 +9,9 @@ http_archive( urls = [ # tag 0.7.0 resolves to commit 6741f733227dc68137512161a5ce6fcf283e3f58 (2019-02-08 18:37:26 +0100) "http://mirror.tensorflow.org/github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", + "http://mirror.tensorflow.org/example.com/nonexistent.txt", + "http://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", + "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", "https://github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", ], ) ``` Re-run the test, and note that it fails with exit code 1 and this text: ``` The following URLs are not properly mirrored: http://mirror.tensorflow.org/example.com/nonexistent.txt Googlers, see http://b/133880558 for further instructions. The following URLs point to the legacy Bazel mirror: WORKSPACE:13: "http://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", WORKSPACE:14: "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/archive/0.7.0.tar.gz", Please update them to use http://mirror.tensorflow.org/ instead. ``` wchargin-branch: mirror-urls-test
wchargin-source: 3f549e6ed919d47770f7977cfb433116c176875a wchargin-branch: mirror-urls-test
wchargin-branch: mirror-urls-test # Conflicts: # .travis.yml
wchargin-source: a3c563dc262c3ecf8567816297cf54b0dc4df065 wchargin-branch: mirror-urls-test
wchargin-source: 9a50cec7bef035729fc363da2c3c7f63d6b83834 wchargin-branch: mirror-urls-test
# shellcheck disable=SC2016 | ||
check_cmd='curl -sfL "$1" >/dev/null || printf "%s\n" "$1"' | ||
# shellcheck disable=SC2016 | ||
exclude='${version}' # as in `ci/download_bazel.sh` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just exclude the specific ci/download_bazel.sh file itself? That seems less brittle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; done.
printf '%s\n' 'The following URLs are not properly mirrored:' | ||
cat "${unresolved_urls_file}" | ||
printf '%s\n' \ | ||
'Googlers, see http://b/133880558 for further instructions.' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More important that instructions for Googlers I think are instructions for what to do as a non-core-TB-team member who gets this error when sending a PR. Probably something like "please comment on your PR asking a TensorBoard core team member to mirror the URLs as per instructions in http://b/133880558".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; done.
wchargin-source: 70eeb29a52fc768ff34982e07060cd83d7d8a6ee wchargin-branch: mirror-urls-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change adds about 1.5s to the CI run for each of the six build jobs
(run concurrently), which seems fine to me.
# shellcheck disable=SC2016 | ||
check_cmd='curl -sfL "$1" >/dev/null || printf "%s\n" "$1"' | ||
# shellcheck disable=SC2016 | ||
exclude='${version}' # as in `ci/download_bazel.sh` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; done.
printf '%s\n' 'The following URLs are not properly mirrored:' | ||
cat "${unresolved_urls_file}" | ||
printf '%s\n' \ | ||
'Googlers, see http://b/133880558 for further instructions.' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; done.
Summary:
This tests that URLs point to the TensorFlow mirror (as opposed to the
Bazel mirror) and that they resolve to valid archives.
Test Plan:
Run
./tensorboard/tools/mirror_urls_test.sh
, and note that it passeswith no output. Then apply the following patch to add some bad URLs:
Re-run the test, and note that it fails with exit code 1 and this text:
Tested on Git v2.15.1, which is what Travis uses.
wchargin-branch: mirror-urls-test