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

Don't vendor exposed API packaged - mux in particular #52

Closed
rlk833 opened this issue Feb 28, 2018 · 9 comments
Closed

Don't vendor exposed API packaged - mux in particular #52

rlk833 opened this issue Feb 28, 2018 · 9 comments

Comments

@rlk833
Copy link

rlk833 commented Feb 28, 2018

Please do not vendor the mux/logger package. This makes it very difficult to use your package. I can't pass in my own mux Router. I have to create a router using the package:

github.com/pivotal-cf/brokerapi/vendor/github.com/gorilla/mux

Or else it is not compatible. The mux Router is not an interface so it can't be just passed in.
Same goes for lager logger.

Actually it is worse. I just tried it and go won't let me use it. It gives an error that I can't use an internal vendor package. So I'm stuck.

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

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

@rlk833
Copy link
Author

rlk833 commented Feb 28, 2018

We use an automated deploy and build for all of our go code. So the suggestion I've seen in other locations for exposed vendored api is not really workable. The workaround is to manually delete the vendored package out of brokerapi after download it. This would be a big pain in our automated process. Everything is just go get and then build.

@nodo
Copy link
Contributor

nodo commented Mar 8, 2018

Hi @rlk833. We could certainly remove vendoring for 3rd party libraries in brokerapi. However, is that what you really need? You will still need to use the gorilla/mux router created in New() or pass one in using AttachRoutes(). Currently we don't support other router other than mux.Router.

@rlk833
Copy link
Author

rlk833 commented Mar 8, 2018

The problem is I can't pass in gorilla/mux router because GO won't accept it. The problem is because you vendor it, GO will only accept a mux router that is created through your internal vendored packaged. But since it is internal vendored I can't create such an instance because that package is hidden. It is a catch-22. Same goes for lager logger.

Anything that you expose as a parameter in the API must be for packages that are not vendored. Otherwise it is impossible to call your API since we can't create objects of the internal vendored type.

@nodo
Copy link
Contributor

nodo commented Mar 8, 2018

Thanks for your quick response.

I am not sure I understand. Please can you give us an example of your code?

You can see how we use the brokerapi package here: https://github.com/pivotal-cf/on-demand-service-broker/blob/master/apiserver/apiserver.go#L45. As you can see we are passing a mux.Router created at line 43.

@rlk833
Copy link
Author

rlk833 commented Mar 8, 2018

The problem is this: I have a router from package

"github.com/gorilla/mux"

.

But the router GO thinks you expose in the API is

"github.com/pivotal-cf/brokerapi/vendor/github.com/gorilla/mux"

This is a completely different package to Go. They are not compatible. I can't pass an instance of my router because it is not of the type of your router.

GO will not let me create a router of an internal vender package like that. And you wouldn't want too either because then I would have to use your internal router everywhere else in my code.

I don't know why your GO is not complaining. Mine is, GO 1.9

your on-demand vendor folder contains brokerapi, but the brokerapi that was vendored DID NOT INCLUDE a vendor folder itself. However doing straight normal go get -d ./... on my code GO pulls your brokerapi AND pulls the packaged vendor folder under brokerapi. This is what causes the conflict. We can't develop because of this.

@rlk833
Copy link
Author

rlk833 commented Mar 8, 2018

Vendor is typically not used on packages which are meant to be used by someone else. They are typically only used for:
a) top level applications that are not included somewhere else
b) internal imported packages that you want to lock down on a version

Any package meant to be imported somewhere else should not vendor any exposed internal vendored packages.

@nodo
Copy link
Contributor

nodo commented Mar 9, 2018

Hey @rlk833,

Fair enough, we have updated brokerapi to not commit the vendor folder. We have also tried to use it as a library and it seems to work now.

Please, let us now if it works for you.

Thanks!

@nodo and @terminatingcode

@rlk833
Copy link
Author

rlk833 commented Mar 9, 2018

Thanks @nodo, that worked great.

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

No branches or pull requests

4 participants