Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Dec 19, 2018

I've moved the package over to the installer with openshift/installer#940 to break the hive <-> installer dependency loop.

While bumping vendor/, I also added non-go and unused-packages to the prune block in Gopkg.toml. This avoids pulling in the installer's newly-vendored Terraform source (openshift/installer#919), but it also removes a whole bunch of previously-vendored stuff. I don't think that will cause any problems, but I guess we'll see in CI.

I've moved the package over to the installer to break the hive <->
installer dependency loop.
…8d17f05

Generated with:

  $ emacs Gopkg.toml  # pin installer to a7468d16b81d
  $ dep ensure

using:

  $ dep version
  dep:
   version     : v0.5.0
   build date  :
   git hash    : 22125cf
   go version  : go1.10.3
   go compiler : gc
   platform    : linux/amd64
   features    : ImportDuringSolve=false

This pulls in the new pkg/destroy/aws (which is descended from our
previous contrib/pkg/awstagdeprovision).

I've also added non-go and unused-packages to the prune block, because
there's no need to ship those around when we don't need them.  For
example, this avoids pulling in the installer's newly-vendored
Terraform source.
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 19, 2018
Catching up with openshift/installer@6f55e673 (terraform/aws: remove
option to use an existing vpc in aws, 2018-11-11,
openshift/installer#654).
@wking
Copy link
Member Author

wking commented Dec 19, 2018

Hrm. Now unit is failing with:

go generate ./pkg/... ./cmd/...
gofmt -w -s pkg contrib
go vet ./pkg/... ./cmd/... ./contrib/...
# github.com/openshift/hive/pkg/apis/hive/v1alpha1
pkg/apis/hive/v1alpha1/zz_generated.deepcopy.go:87:8: (*in).DeepCopyInto undefined (type *ipnet.IPNet has no field or method DeepCopyInto)

I think I need more background on why we have the install-config types under v1alpha1. @dgoodwin, can you give some more background around 88ebffb (#2)? I see the pull-request topic post mentions converting IPNet to strings. I can go back though and do that here, but is this approach stale now that we're moving towards config.openshift.io types (openshift/installer#680)? What are these Kube bindings for?

@wking wking force-pushed the vendor-aws-destroy-logic branch from 2a66f36 to 02ff8fa Compare December 19, 2018 08:18
Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR
validation, 2018-11-27, openshift/installer#711).

We can't convert the v1alpha1 type to an *IPNet, because it doesn't
have DeepCopyInto methods [1]:

  go vet ./pkg/... ./cmd/... ./contrib/...
  # github.com/openshift/hive/pkg/apis/hive/v1alpha1
  pkg/apis/hive/v1alpha1/zz_generated.deepcopy.go:87:8: (*in).DeepCopyInto undefined (type *ipnet.IPNet has no field or method DeepCopyInto)

We used to define those methods in the installer, but stopped in
openshift/installer@ca6f6195 (Revert "pkg/ipnet: Add DeepCopy and
DeepCopyInto for IPNet", 2018-09-20, openshift/installer#295).

[1]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_hive/144/pull-ci-openshift-hive-master-unit/253/build-log.txt
@wking wking force-pushed the vendor-aws-destroy-logic branch from 02ff8fa to f046c35 Compare December 19, 2018 08:22
@dgoodwin
Copy link
Contributor

dgoodwin commented Dec 19, 2018

Looks like you got this solved but for context if you recall way back in the arch calls a few months ago we had multiple discussions on using InstallConfig types in Kube. (Hive is driven by CRDs, the main one being ClusterDeployment which embeds something very similar to, but different than, InstallConfig. We decided on call the best path would be to fork but keep them as similar as possible given the different needs of each.

@dgoodwin
Copy link
Contributor

This looks good, thanks @wking we had a card for this and now we can close it. :) I've tested creating a cluster deployment and everything looks good.

CC @joelddiaz no need to move deprovision code to installer now.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2018
@openshift-merge-robot openshift-merge-robot merged commit 464c1c1 into openshift:master Dec 19, 2018
@wking wking deleted the vendor-aws-destroy-logic branch September 10, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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