Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

Conversation

@mxinden
Copy link
Contributor

@mxinden mxinden commented Oct 4, 2017

The smoke.sh bash script is fully replaced by the Ruby scripts and
thereby not needed anymore. In addition the aws-vpc.tfvars file is
replaced by aws-vpc-internal.tfvars.json.

The smoke.sh bash script is fully replaced by the Ruby scripts and
thereby not needed anymore. In addition the aws-vpc.tfvars file is
replaced by aws-vpc-internal.tfvars.json.
Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. All the tests were using .tfvars.json files, so that one file was clearly out of place.

@mxinden
Copy link
Contributor Author

mxinden commented Oct 4, 2017

@coreosbot ok to test

1 similar comment
@mxinden
Copy link
Contributor Author

mxinden commented Oct 5, 2017

@coreosbot ok to test

@mxinden mxinden merged commit 760b243 into coreos:master Oct 5, 2017
wking added a commit to wking/openshift-installer that referenced this pull request Jul 9, 2018
We haven't set $WORKSPACE since 1dea5c8 (tests: Remove unused
smoke.sh + tfvars file, 2017-10-04, coreos/tectonic-installer#2036),
so there's no longer a need for the --workspace-dir options.  Users
who care where the scratch files live can set $TMPDIR:

  $ man 7 environ | grep TMPDIR | sed 's/  */ /g' | cut -b -67
   * TMPDIR influences the path prefix of names created by tempnam(3)

I'm still calling readlink on the mktemp output in case $TMPDIR (or
/tmp, if $TMPDIR is unset) is a symlink.

I'm also fixing --config-file, --exclude-file, and --tag-file.
Previously we were using:

  CONFIG_FILE="/tmp/config/$(basename "$config_file")"

and similar.  But inside the container, /tmp/config is coming from the
$tmp_dir volume mount.  And when --config-file was set, we weren't
writing the referenced content into $tmp_dir.  Now we always write the
content into $tmp_dir, regardless of whether the content is
user-supplied or the script's default.

Also avoid some parallel-call races by avoiding a shared /tmp/config
(or ${workspace}/config).  With the old approach, the trap rm call for
one invocation could blow away a /tmp/config used by another
invocation.  With this commit, we use mktemp to give a secure, unique
$tmp_dir.  And once we have that, we can hard-code the paths to the
config, tag, and exclude files inside $tmp_dir.
wking added a commit to wking/openshift-installer that referenced this pull request Aug 3, 2018
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 added a commit to wking/openshift-installer that referenced this pull request Aug 24, 2018
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants