Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: integrate with latest openshift version #195

Merged
merged 2 commits into from
Apr 6, 2019

Conversation

ironcladlou
Copy link
Contributor

@ironcladlou ironcladlou commented Apr 3, 2019

  • Upgrade to latest openshift/api and libary-go
  • Upgrade to kube 1.13
  • Upgrade to latest compatible controller-runtime

Fixes openapi spec generation, which as of the previous version was not picking
up field descriptions.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Apr 3, 2019
@@ -1,10 +1,6 @@
# Force dep to vendor the code generators, which aren't imported just used at dev time.
# Picking a subpackage with Go code won't be necessary once https://github.com/golang/dep/pull/1545 is merged.
required = [
"k8s.io/code-generator/cmd/defaulter-gen",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these code generators being removed b/c golang/dep#1545 merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest commit cleans up the entire file including the required stanza. We still need some stuff in required for non-imported code generation, but the subpackage note isn't relevant to us.

Gopkg.toml Outdated
@@ -38,24 +34,34 @@ required = [

[[constraint]]
name = "github.com/openshift/library-go"
revision = "fc4b2d5cbcf0ab855422e340c78fe7a53a38bee3"
revision = "9067dee609b10991fa6c26b5d51fe095b2331027"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for using this rev instead of latest (dab26bb3a8dc7fccde7227194af755bbff30ce5d)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo on my part, good catch!

@ironcladlou ironcladlou force-pushed the update-client branch 2 times, most recently from 3081209 to b10985e Compare April 3, 2019 23:04
@ironcladlou
Copy link
Contributor Author

@ironcladlou ironcladlou force-pushed the update-client branch 2 times, most recently from 5cc98f4 to 8506f80 Compare April 4, 2019 14:44
@ironcladlou ironcladlou changed the title WIP: deps: bump WIP: deps: integrate with latest openshift version Apr 4, 2019
@ironcladlou ironcladlou changed the title WIP: deps: integrate with latest openshift version deps: integrate with latest openshift version Apr 4, 2019
@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 Apr 4, 2019
@ironcladlou
Copy link
Contributor Author

Need a resolution to kubernetes-sigs/controller-runtime#383.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2019
@ironcladlou
Copy link
Contributor Author

@openshift/sig-network-edge

I don't want to wait for this. I'd like to carry the patched Source in our tree (e.g. in pkg/util) and duplicate the dependent controller-runtime packages to make it work. When/if upstream merges, we can remove our copy.

If there are no objections, I'll do this tomorrow (April 5).

@ironcladlou
Copy link
Contributor Author

I figured, why wait. I went ahead and pushed the local Source implementation. If upstream patches it'll be trivial to delete our local version.

This is ready for review.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2019
@ironcladlou
Copy link
Contributor Author

/test e2e-aws

Upgrade to the latest libraries compatible with openshift/api and library-go.
@ironcladlou
Copy link
Contributor Author

kubernetes-sigs/controller-runtime#383 merged last night so I fixed up the PR to use it!

@ironcladlou
Copy link
Contributor Author

/retest

@Miciah
Copy link
Contributor

Miciah commented Apr 5, 2019

Typo in the commit message: "libary-go".

@ironcladlou
Copy link
Contributor Author

Typo in the commit message: "libary-go".

Fixed

//
// If upstream accepts the patch, this package can be deleted and replaced by
// upstream code.
package source
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like your forgot to remove this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have failed to isolate those changed in the deleted commit. Thanks!

* Upgrade to latest openshift/api and library-go
* Upgrade to kube 1.13
* Upgrade to latest compatible controller-runtime

Fixes openapi spec generation, which as of the previous version was not picking
up field descriptions.
@Miciah
Copy link
Contributor

Miciah commented Apr 5, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2019
Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah, pravisankar

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Miciah,ironcladlou,pravisankar]

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

@ironcladlou
Copy link
Contributor Author

/retest

1 similar comment
@ironcladlou
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 4b758b6 into openshift:master Apr 6, 2019
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.

7 participants