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

Make external API a separate module #4298

Merged
merged 5 commits into from
Sep 11, 2020
Merged

Conversation

danielsdeleo
Copy link
Contributor

πŸ”© Description: What code changed, and why?

Make the api/external directory a separate golang module. When using golang modules, when you depend on any package within a module, you depend on that module and therefore also all of that module's dependencies. Currently all of Automate is one big module which is nice for our internal dependency management but for external users it means you have to take on a lot of deps even if you only intend to use a few structs from api/external directory.

At present the goal is to support a project that is integrating some Chef Workstation functionality with Chef Automate, but this would be helpful to anyone who wishes to write code using Automate's API in go.

⛓️ Related Resources

πŸ‘ Definition of Done

πŸ‘Ÿ How to Build and Test the Change

βœ… Checklist

πŸ“· Screenshots, if applicable

@netlify
Copy link

netlify bot commented Sep 2, 2020

Deploy preview for chef-automate ready!

Built with commit 0f190e3

https://deploy-preview-4298--chef-automate.netlify.app

@danielsdeleo danielsdeleo force-pushed the dan/separate-api-module branch 3 times, most recently from c1a8a73 to 38cb682 Compare September 2, 2020 23:53
@stevendanna
Copy link
Contributor

I wonder if this is a time to re-look at our vendoring in this repository. Having to re-vendor our own code anytime we make a change to it feels like it will trip people up regularly.

@srenatus
Copy link
Contributor

srenatus commented Sep 3, 2020

@stevendanna you're suggesting that we stop vendoring, and rely completely on go modules instead? It has worked fine for the other project, I'd say. πŸ‘

.bldr.toml Outdated
@@ -439,6 +439,7 @@ paths = [
"vendor/github.com/grpc-ecosystem/grpc-gateway/*",
"vendor/github.com/grpc-ecosystem/grpc-opentracing/*",
"vendor/github.com/hashicorp/hcl/*",
"vendor/github.com/josharian/intern/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ€” I wonder how this got dragged in

@stevendanna
Copy link
Contributor

you're suggesting that we stop vendoring, and rely completely on go modules instead?

I think we should consider it, yes. We started vendoring when using dep which was substantially slower without vendoring.

In my views, if we stop vendoring, the cons are:

  • Updating dependencies no longer provides an easy way to review what is changing in those dependencies.
  • We'll likely have to give up on rebuilding all affected components when a dependency changes. This means occasionally we could have build-breaks caused by a previous dependency update. (we might be able to do something here with changes to our existing tooling).
  • More network resources required during the build (likely can be mitigated with some caching)

The pros are:

  • Fewer situations where people need to regenerate bldr.toml.
  • Likely makes it easier to split out more modules over time if we care to.
  • Less work when updating a dependency

@srenatus
Copy link
Contributor

srenatus commented Sep 3, 2020

Updating dependencies no longer provides an easy way to review what is changing in those dependencies.

Yeah, it's still in the go.{sum,mod} files, but you won't see the code right away.

I agree with your pros and cons there, thanks for summing this up. :)

@danielsdeleo danielsdeleo force-pushed the dan/separate-api-module branch 3 times, most recently from b927f2f to 39d4223 Compare September 3, 2020 18:33
@danielsdeleo danielsdeleo mentioned this pull request Sep 4, 2020
4 tasks
Signed-off-by: Daniel DeLeo <dan@chef.io>
Signed-off-by: Daniel DeLeo <dan@chef.io>
Signed-off-by: Daniel DeLeo <dan@chef.io>
Signed-off-by: Daniel DeLeo <dan@chef.io>
Signed-off-by: Daniel DeLeo <dan@chef.io>
@danielsdeleo
Copy link
Contributor Author

Update: in #4314 we have removed vendoring, so now this change is mostly transparent for an Automate developer. I checked that you can update a proto in api/external, compile it, and then use it right away (no need to revendor or git commit or anything). Updating deps that appear in both modules may be confusing, and I'm not sure what I could do about that (or rather, I would like to have some practical experience of the problem before attempting to fix). So I think this is ready to go now.

@danielsdeleo danielsdeleo removed the WIP label Sep 9, 2020
go.mod Show resolved Hide resolved
Copy link
Contributor

@lancewf lancewf left a comment

Choose a reason for hiding this comment

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

Everything worked the same for me.

@danielsdeleo danielsdeleo merged commit d63778a into master Sep 11, 2020
@danielsdeleo danielsdeleo deleted the dan/separate-api-module branch September 11, 2020 16:37
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

Successfully merging this pull request may close these issues.

None yet

4 participants