Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Aug 16, 2018

The smoke target was a lot like the Gazelle-maintained go_default_test target, except that it's not automatically maintained. Make maintenance easier by pointing the smoke_tests alias straight at the automatically-maintained target. Generated with:

$ rm smoke/tests/BUILD.bazel
$ bazel run //:gazelle
$ emacs smoke/tests/BUILD.bazel  # add visibility

Adding the visibility property avoids:

$ bazel run smoke_tests
ERROR: /home/trking/.local/lib/go/src/github.com/openshift/installer/BUILD.bazel:45:1: target '//tests/smoke:go_default_test' is not visible from target '//:smoke_tests'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: Analysis of target '//:smoke_tests' failed; build aborted: Analysis of target '//:smoke_tests' failed; build aborted
INFO: Elapsed time: 0.124s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded)
FAILED: Build did NOT complete successfully (1 packages loaded)

Running the smoke tests is also fairly orthogonal to building tarballs, so I've removed the smoke-test docs from Documentation/dev/build.md.

The last test_vars consumer was removed in 7296010 (coreos/tectonic-installer#3067), so I don't think its removal will be a problem. And because smoke.sh was removed in 1dea5c8 (coreos/tectonic-installer#2036), I've just removed the whole tests/smoke/aws tree. And without that tree to explain, I've dropped the associated section from the smoke README as well.

This PR has two commits:

  1. Just a //:gazelle run, so you can see what it wants to change vs. the current master.
  2. The smoke-dropping commit, so you can see what is being changed because of that.

Spun off from #94 to get smaller, more-easily-reviewed chunks.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 16, 2018
@wking wking mentioned this pull request Aug 16, 2018
@wking wking force-pushed the drop-smoke-target branch from 95cc31a to 17f5589 Compare August 24, 2018 05:30
@wking
Copy link
Member Author

wking commented Aug 24, 2018

Rebased around #88 with 95cc31a -> 17f5589.

wking added 2 commits August 24, 2018 07:25
Generated with:

  $ bazel run //:gazelle

Using:

  $ bazel version
  Build label: 0.15.2- (@Non-Git)
  Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
  Build time: Tue Jul 17 13:32:28 2018 (1531834348)
  Build timestamp: 1531834348
  Build timestamp as int: 1531834348
The 'smoke' target is a lot like the Gazelle-maintained
go_default_test target, except that it's not automatically maintained.
Make maintenance easier by pointing the smoke_tests alias straight at
the automatically-maintained target.  Generated with:

  $ rm smoke/tests/BUILD.bazel
  $ bazel run //:gazelle
  $ emacs smoke/tests/BUILD.bazel  # add visibility

Adding the visibility property avoids:

  $ bazel run smoke_tests
  ERROR: /home/trking/.local/lib/go/src/github.com/openshift/installer/BUILD.bazel:45:1: target '//tests/smoke:go_default_test' is not visible from target '//:smoke_tests'. Check the visibility declaration of the former target if you think the dependency is legitimate
  ERROR: Analysis of target '//:smoke_tests' failed; build aborted: Analysis of target '//:smoke_tests' failed; build aborted
  INFO: Elapsed time: 0.124s
  INFO: 0 processes.
  FAILED: Build did NOT complete successfully (1 packages loaded)
  FAILED: Build did NOT complete successfully (1 packages loaded)

Running the smoke tests is also fairly orthogonal to building
tarballs, so I've removed the smoke-test docs from
Documentation/dev/build.md.

The last test_vars consumer was removed in 7296010 (frontend: Remove
frontend code and backend API code, 2018-03-06,
coreos/tectonic-installer#3067), so I don't think its removal will be
a problem.  And because smoke.sh was removed in 1dea5c8 (tests:
Remove unused smoke.sh + tfvars file, 2017-10-04,
coreos/tectonic-installer#2036), I've just removed the whole
tests/smoke/aws tree.  And without that tree to explain, I've dropped
the associated section from the smoke README as well.
@wking wking force-pushed the drop-smoke-target branch from 17f5589 to 71c27f6 Compare August 24, 2018 14:25
@wking
Copy link
Member Author

wking commented Aug 24, 2018

Rebased around #162 with 17f5589 -> 71c27f6.

@wking
Copy link
Member Author

wking commented Aug 24, 2018

All green here. Anyone free to take a look? @yifan-gu, @abhinavdahiya?

@crawford
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit d649b2b into openshift:master Aug 24, 2018
@wking wking deleted the drop-smoke-target branch August 24, 2018 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants