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

frontend: Remove GUI for Track 2 #3067

Merged
merged 2 commits into from
Mar 16, 2018
Merged

frontend: Remove GUI for Track 2 #3067

merged 2 commits into from
Mar 16, 2018

Conversation

kyoto
Copy link
Contributor

@kyoto kyoto commented Mar 5, 2018 •
edited by squat
Loading

Remove GUI for Track 2.

Removes all frontend code and backend API code and the associated tests.

Does not include removing any Go packages that may now be unused.

Restoring the GUI will require migrating to the YAML Tectonic config file format. Some of that work was done in #2884.

cc: @sym3tri, @robszumski, @squat, @cpanato

@coreosbot
Copy link

Can one of the admins verify this patch?

@cpanato cpanato mentioned this pull request Mar 5, 2018
Closed
cpanato
cpanato reviewed Mar 5, 2018
View reviewed changes
Copy link
Contributor

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

:-(

squat
squat reviewed Mar 5, 2018
View reviewed changes
tests/README.md Outdated
- Add the `run-gui-tests` GitHub label

### FAQ
- *I am not able to add labels, what should I do?*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why eliminate this block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These FAQs are actually repeated in other sections of the doc.

squat
squat reviewed Mar 5, 2018
View reviewed changes
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.

Let's not merge this until the PR to remove all the frontend code is up and then we can merge these both in a row.

@kyoto kyoto force-pushed the disable-gui-tests branch from 8e766b3 to 652356f Compare March 6, 2018 07:23
@kyoto kyoto changed the title tests/frontend: Disable frontend unit tests and end-to-end tests frontend: Remove GUI for Track 2 Mar 6, 2018
@kyoto
Copy link
Contributor Author

kyoto commented Mar 6, 2018

ok to test

@squat
Copy link
Contributor

squat commented Mar 8, 2018

@kyoto can we add another commit that re-vendors? I can do this for you if you like, but we should really eliminate all unused vendored packages as part of this

@kyoto kyoto force-pushed the disable-gui-tests branch from 652356f to ef59061 Compare March 9, 2018 16:01
@kyoto
Copy link
Contributor Author

kyoto commented Mar 9, 2018

Rebased

mxinden added a commit to mxinden/tectonic-installer that referenced this pull request Mar 13, 2018
@mxinden
tests: Remove GUI tests
556691e 
More in depth cleanup will happen in PR 3068 [1]. This quick-fix
unblocks current master failures.

[1] coreos#3067
@mxinden mxinden mentioned this pull request Mar 13, 2018
Merged
mxinden added a commit to mxinden/tectonic-installer that referenced this pull request Mar 13, 2018
@mxinden
tests: Remove GUI tests
9da7b95 
More in depth cleanup will happen in PR 3067 [1]. This quick-fix
unblocks current master failures.

[1] coreos#3067
@alexsomesan
Copy link
Contributor

@squat @kyoto
Is this change still in need of a followup remove of the remaining unused code? I'd like to merge soonish to be able to depend on the frontend not being there.

@squat
Copy link
Contributor

squat commented Mar 14, 2018

@alexsomesan: @kyoto is taking some time off. I’m working on cleaning up the last remaining vendor files. Should be done tomorrow.

@squat squat force-pushed the disable-gui-tests branch from ef59061 to c2a4759 Compare March 15, 2018 13:56
@squat
Copy link
Contributor

squat commented Mar 15, 2018

@mxinden @enxebre @alexsomesan PTAL. All unneeded vendored go code is now removed.

@cpanato
Copy link
Contributor

cpanato commented Mar 15, 2018

@squat need to remove the bazel test installer/frontend:unit --test_output=all from jenkinsfile

enxebre
enxebre reviewed Mar 15, 2018
View reviewed changes
installer/README.md Outdated
@@ -29,52 +14,14 @@ Get a [license](https://account.coreos.com) and follow the guides to create Tect

- [Go 1.8](https://golang.org/doc/install)
- [Nodejs >=8.x](https://nodejs.org/en/download/)
Copy link
Member

Choose a reason for hiding this comment

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

we can remove Nodejs as well , can't we?

@enxebre
Copy link
Member

enxebre commented Mar 15, 2018 •
edited
Loading

This will need cleanup as well https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/images/tectonic-builder/Dockerfile#L48, actually are we using this image anymore? cc @cpanato @mxinden

@cpanato
Copy link
Contributor

cpanato commented Mar 15, 2018

and the node as well https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/images/tectonic-builder/Dockerfile#L66

@squat squat force-pushed the disable-gui-tests branch 3 times, most recently from 6c1c1cf to 3bc7ce9 Compare March 15, 2018 16:00
@squat
Copy link
Contributor

squat commented Mar 15, 2018

ok to test

@enxebre
Copy link
Member

enxebre commented Mar 16, 2018

All the nodejs related stuff in bazel WORKSPACE can go away https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/WORKSPACE#L21

@enxebre
Copy link
Member

enxebre commented Mar 16, 2018 •
edited
Loading

Nodejs related here can be removed https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/bill-of-materials.json#L363

@enxebre
Copy link
Member

enxebre commented Mar 16, 2018 •
edited
Loading

this wont be needed anymore https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/tests/smoke/bare-metal/packet/main.tf#L119 but could be removed as a part of a follow up for keep removing obsolete bits

@enxebre
Copy link
Member

enxebre commented Mar 16, 2018

This can be removed https://github.com/kyoto/tectonic-installer/blob/disable-gui-tests/installer/scripts/license-gen/collect_project_licenses.sh#L174

@squat
Copy link
Contributor

squat commented Mar 16, 2018

The last two things are unrelated to the GUI so let’s do that separately. I’ll regenerate the BOM and fix the WORKSPACE. Thanks for finding those.

@squat squat force-pushed the disable-gui-tests branch from 3bc7ce9 to 38d46bc Compare March 16, 2018 09:27
@mxinden
Copy link
Contributor

mxinden commented Mar 16, 2018

restarted aws tests

@squat squat force-pushed the disable-gui-tests branch from 38d46bc to 7296010 Compare March 16, 2018 11:31
@squat squat merged commit 34c0377 into coreos:master Mar 16, 2018
wking added a commit to wking/openshift-installer that referenced this pull request Jul 25, 2018
@wking
images/tectonic-builder/Dockerfile: Drop license-bill-of-materials
ab7d937 
The last code consuming this executable was removed in 7296010
(frontend: Remove frontend code and backend API code, 2018-03-06,
coreos/tectonic-installer#3067).
@wking wking mentioned this pull request Jul 25, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Aug 3, 2018
@wking
tests/smoke/BUILD.bazel: Drop the 'smoke' target
35c0362 
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 mentioned this pull request Aug 16, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Aug 24, 2018
@wking
tests/smoke/BUILD.bazel: Drop the 'smoke' target
71c27f6 
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 mentioned this pull request Sep 5, 2018
Merged
@wking wking mentioned this pull request Sep 10, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Sep 10, 2018
@wking
builder-run: Drop unused script
acd2791 
This script is from f7103ba (build: add back builder-run utility
script, 2017-05-09, coreos/tectonic-installer#609), but doesn't seem
to have ever been used anywhere.  With installer/scripts gone since
7296010 (frontend: Remove frontend code and backend API code,
2018-04-06, coreos/tectonic-installer#3067), I think we can just drop
builder-run entirely.
frobware pushed a commit to frobware/installer that referenced this pull request Sep 17, 2018
@wking @frobware
images/tectonic-builder/Dockerfile: Drop license-bill-of-materials
98b5c83 
The last code consuming this executable was removed in 7296010
(frontend: Remove frontend code and backend API code, 2018-03-06,
coreos/tectonic-installer#3067).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@squat squat squat left review comments

@cpanato cpanato cpanato left review comments

@enxebre enxebre enxebre left review comments

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
platform/aws run-gui-tests run-smoke-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kyoto @coreosbot @squat @alexsomesan @cpanato @enxebre @mxinden