Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Updating latest depencies #150

Merged
merged 2 commits into from
Mar 27, 2018

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented Mar 27, 2018

First commit auto generated by running glide update --strip-vendor.

Had to clear cache as per @G-Harmon's suggestion: Masterminds/glide#592 (comment)

Also removed the kubernetes pin that was added in #93

Second commit is to fix the build using latest dependencies (renames and code moves).
It has following changes that were required to fix the build:

  • Rename ServicePort.Port to Nodeport: Rename Port to NodePort kubernetes/ingress-gce#128
  • Update the return argument from ListGlobalForwardingRules to be []*ForwardingRule instead of ForwardingRuleList
  • Update the import path for constants from ingress-gce like AppProtocol, ProtocolHTTP to be pkg/annotations instead of utils.
  • Update the usage of annotations library from ingress-gce to use annotations.FromIngress(ing) instead of annotations.IngAnnotations(ing.ObjectMeta.Annotations) (similar change for services)

Trigger for getting latest change: kubernetes/ingress-gce#169, kubernetes/ingress-gce#165.

cc @G-Harmon @csbell @madhusudancs

@nikhiljindal
Copy link
Contributor Author

Verified that go run test/e2e/e2e.go is still green with these changes :)

@madhusudancs Will be great if you can take a look since I am not sure if @csbell and @G-Harmon will get to.

@@ -59,7 +59,7 @@ func (b *BackendServiceSyncer) EnsureBackendService(lbName string, ports []ingre
var err error
ensuredBackendServices := BackendServicesMap{}
for _, p := range ports {
be, beErr := b.ensureBackendService(lbName, p, hcMap[p.Port], npMap[p.Port], igLinks, forceUpdate)
be, beErr := b.ensureBackendService(lbName, p, hcMap[p.NodePort], npMap[p.NodePort], igLinks, forceUpdate)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated to latest dependencies. Is it intentional? The linked triggers don't clarify this either.

Could you please explain this change in the PR description or link to the relevant trigger? Same question for all the Port -> NodePort changes below. I think I am missing context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more context. This is because the struct that we are using from ingress-gce renamed Port to NodePort: kubernetes/ingress-gce#128

@nikhiljindal
Copy link
Contributor Author

To add more context, the second commit has following changes that were required to fix the build:

  • Rename ServicePort.Port to Nodeport: Rename Port to NodePort kubernetes/ingress-gce#128
  • Update the return argument from ListGlobalForwardingRules to be []*ForwardingRule instead of ForwardingRuleList
  • Update the import path for constants from ingress-gce like AppProtocol, ProtocolHTTP to be pkg/annotations instead of utils.
  • Update the usage of annotations library from ingress-gce to use annotations.FromIngress(ing) instead of annotations.IngAnnotations(ing.ObjectMeta.Annotations) (similar change for services)

These are all due to changes to ingress-gce code that we are using.

@madhusudancs
Copy link
Contributor

Thanks for the clarification. It would be great if you can copy that comment to your original PR description and/or the second commit so others who might have to bisect the commits get better visibility.

@madhusudancs
Copy link
Contributor

/lgtm

@nikhiljindal
Copy link
Contributor Author

Sure updated the PR description as well. Thanks!

@nikhiljindal nikhiljindal merged commit 345ee76 into GoogleCloudPlatform:master Mar 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants