Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Aug 1, 2018

Red Hat no longer requires a Tectonic license now that this installer is being used to launch OpenShift clusters.

There's also a k8s.io/metrics pin in the smoke-test glide.yaml and a revendor to drop the no-longer-needed jwt-go. Details in the commit messages.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 1, 2018
@wking wking changed the title *: Drop the Tectonic license wip: *: Drop the Tectonic license Aug 1, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2018
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this package being vendored now? I don't see anyone importing it:

$  git grep '"k8s.io/api/apps/v1"' tests/smoke | cat
tests/smoke/vendor/k8s.io/api/apps/v1/BUILD.bazel:    importpath = "k8s.io/api/apps/v1",
tests/smoke/vendor/k8s.io/api/apps/v1/doc.go:package v1 // import "k8s.io/api/apps/v1"

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this package being vendored now?

Because we'd been using glide-vc --use-lock-file .... I've added another commit to drop the --use-lock-file suggestion, rerun the vendoring without the option, and now we're no longer adding this file.

@wking wking force-pushed the drop-license branch 2 times, most recently from 11d343b to c85002c Compare August 1, 2018 18:50
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2018
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 27, 2018
@wking
Copy link
Member Author

wking commented Aug 27, 2018

I've rebased this onto master with c85002c -> acf1bb7. I've left the Go dependencies alone for now, because I was having difficulty updating them without also updating the smoke-test dependencies. And I was having trouble updating the smoke-test dependencies on their own in #94. Since I'm not clear how much longer the smoke tests will be around (maybe openshift/release#1271 is the road forward on that front?), I'm going to focus on the code changes for now and circle back to the vendor changes later.

@wking wking force-pushed the drop-license branch 2 times, most recently from 0cc5b35 to 0807ef9 Compare August 28, 2018 22:31
wking added 2 commits August 30, 2018 15:01
Red Hat no longer requires a Tectonic license now that this installer
is being used to launch OpenShift clusters.
Regenerate with:

  $ dot -Tsvg Documentation/design/resource_dep.dot >Documentation/design/resource_dep.svg
  $ dot -V
  dot - graphviz version 2.30.1 (20170916.1124)
@wking
Copy link
Member Author

wking commented Aug 30, 2018

The smoke test errors have been timing out waiting for the tectonic-stats-emitter pod:

smoke_test.go:113: retrying in 3s
cluster_test.go:100: pod tectonic-system/tectonic-stats-emitter-d87f669fd-7ch7q not running
smoke_test.go:112: failed with error: pods are not all ready
smoke_test.go:113: retrying in 3s
cluster_test.go:69: Timed out waiting for pods to be ready.

I'm not sure if that's something I broke or not.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2018
@wking wking force-pushed the drop-license branch 2 times, most recently from e018790 to 485b3f4 Compare August 30, 2018 23:56
@wking
Copy link
Member Author

wking commented Aug 31, 2018

With 485b3f4, the smoke-test errors were:

smoke_test.go:113: retrying in 3s
cluster_test.go:100: pod tectonic-system/tectonic-channel-operator-786b7844bd-sfxgp not running
smoke_test.go:112: failed with error: pods are not all ready
smoke_test.go:113: retrying in 3s
cluster_test.go:69: Timed out waiting for pods to be ready.

tectonic-stats-emitter requires the Tectonic license to extract the
customer's accountID and send stats about how they use Tectonic.  But
Red Hat no longer requires a Tectonic license now that this installer
is being used to launch OpenShift clusters.

This commit updates our Tectonic operators from c56e9aa2 -> 436b1b43,
which is just dropping the stats emitter from those operators
(primarily the utility operator).  It also removes our local stats
references.

The current plan for stats is to handle them with a cluster-monitoring
operator running a telemetry client, but we can plug that back in
later (and it won't need a license).
Generated with:

  $ glide get --test --strip-vendor github.com/stretchr/testify/assert
  [INFO]Preparing to install 1 package.
  [INFO]Attempting to get package github.com/stretchr/testify/assert
  [INFO]--> Gathering release information for github.com/stretchr/testify
  [INFO]The package github.com/stretchr/testify appears to have Semantic Version releases (http://semver.org).
  [INFO]The latest release is v1.2.2. You are currently not using a release. Would you like
  [INFO]to use this release? Yes (Y) or No (N)
  y
  [INFO]The package github.com/stretchr/testify appears to use semantic versions (http://semver.org).
  [INFO]Would you like to track the latest minor or patch releases (major.minor.patch)?
  [INFO]The choices are:
  [INFO] - Tracking minor version releases would use '>= 1.2.2, < 2.0.0' ('^1.2.2')
  [INFO] - Tracking patch version releases would use '>= 1.2.2, < 1.3.0' ('~1.2.2')
  [INFO] - Skip using ranges
  [INFO]For more information on Glide versions and ranges see https://glide.sh/docs/versions
  [INFO]Minor (M), Patch (P), or Skip Ranges (S)?
  m
  [INFO]--> Adding github.com/stretchr/testify to your configuration with the version ^1.2.2
  ...
  $ rm -rf glide.lock vendor
  $ glide install --strip-vendor
  $ glide-vc --use-lock-file --no-tests --only-code
  $ bazel run //:gazelle

using:

  $ glide --version
  glide version v0.13.1
  $ (cd $GOPATH/src/github.com/sgotti/glide-vc && git describe)
  v0.1.0-2-g6ddf6ee
  $ bazel version
  Build label: 0.16.1- (@Non-Git)
  Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
  Build time: Mon Aug 13 16:42:29 2018 (1534178549)
  Build timestamp: 1534178549
  Build timestamp as int: 1534178549

The changes look reasonable, with the possible exception of client-go:

  $ cd $GOPATH/src/k8s.io/client-go
  $ git describe 78700dec6369ba22221b72770783300f143df150
  v6.0.0
  $ git describe 7d04d0e2a0a1a4d4a1cd6baa432a2301492e4e65
  v8.0.0

The v6 hash came into this repo with da6286f (vendor: Update vendor
for installconfig generation, 2018-08-23, openshift#160), so I'm not clear on
why it was pulling in the 2017-12-06 v6 instead of the 2018-06-28 v8.
I guess CI will let us know if anything breaks ;).
@wking wking changed the title wip: *: Drop the Tectonic license *: Drop the Tectonic license Aug 31, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2018
@wking
Copy link
Member Author

wking commented Aug 31, 2018

All green 🎉

/assign @abhinavdahiya

@@ -0,0 +1,25 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
Copy link
Contributor

Choose a reason for hiding this comment

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

These files shouldn't exist. Right? We don't need bazel for new installer binary.

@abhinavdahiya
Copy link
Contributor

There are bazel build files in new installer binary packages? It would be nice if don't have them. Otherwise /lgtm @wking

@wking
Copy link
Member Author

wking commented Aug 31, 2018

There are bazel build files in new installer binary packages?

bazel run //:gazelle (which I ran here) adds them.

@abhinavdahiya
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, 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:
  • OWNERS [abhinavdahiya,wking]

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 a47aeb1 into openshift:master Aug 31, 2018
@wking wking deleted the drop-license branch August 31, 2018 22:22
stbenjam pushed a commit to stbenjam/installer that referenced this pull request Feb 10, 2021
Bug 1913047: Remove intermediate CO progressing state
clnperez added a commit to clnperez/installer that referenced this pull request Dec 15, 2021
conflict in stages.go. openshift and ovirt were also added upstream. we added powervs.
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.

5 participants