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

expose internal_route_vip_range as link #116

Closed

Conversation

ameowlia
Copy link
Member

[#162515686]

Signed-off-by: Alex Standke astandke@pivotal.io

Thanks for contributing to the capi_release. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

Expose internal_route_vip_range as bosh link

  • An explanation of the use cases your change solves

We wanna use it so it's only configured in one place (and not hardcoded 😐)

  • Links to any other associated PRs

Nada

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the develop branch

  • I have run CF Acceptance Tests on bosh lite

[#162515686]

Signed-off-by: Alex Standke <astandke@pivotal.io>
@cfdreddbot
Copy link

✅ Hey ameowlia! The commit authors and yourself have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/162746053

The labels on this github issue will be updated when the story is started.

@XanderStrike
Copy link

wow great change!

@@ -103,6 +103,10 @@ provides:
- cc.internal_service_hostname
- cc.tls_port
- cc.mutual_tls.ca_cert
- name: cloud_controller_networking_info
Copy link
Member

Choose a reason for hiding this comment

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

Can we tweak the name a bit to make it more clear that this is for container networking and not general "I want to talk with the Cloud Controller" networking -- I've seen this cause confusion in the past with some of our links and their consumers.

Maybe something like:

  • cloud_controller_container_networking_info
  • cloud_controller_app_networking_info
  • cloud_controller_runtime_networking_info
  • Something even better 🥇

@tcdowney
Copy link
Member

tcdowney commented Jan 2, 2019

@ssisil

@zrob would sometimes consider the public links we provide and their naming part of the capi-release product surface area. This PR might be something you're interested in.

I left some suggestions for renaming the link by the way and you're welcome to weigh in. 👍

@tcdowney
Copy link
Member

tcdowney commented Jan 2, 2019

@ameowlia @XanderStrike

I think this link makes a lot of sense, but I'd like for it to be renamed so that consumers have a better idea of what kind of networking we're talking about here. Thanks!

@tcdowney
Copy link
Member

tcdowney commented Jan 2, 2019

Actually this looks like it was merged already:
520aa68

I still think we should rename it before we cut a capi-release, though. @ameowlia have you already made changes to cf-networking-release for this?

@XanderStrike
Copy link

Thanks for your review! We don't consume the link yet because we were waiting on y'all to merge. Would it be easier for you guys to change the name real quick? That way we don't have to go through the PR flow again.

tcdowney added a commit that referenced this pull request Jan 2, 2019
This will make the intent of the link more clear that
it's to be used for container networking related properties

See PR #116 for more information.

[finishes #162746053]
@tcdowney
Copy link
Member

tcdowney commented Jan 2, 2019

This PR was merged in 520aa68 and updated in 9ebfc4f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants