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

Remove proto dep external->interservice #4278

Merged
merged 1 commit into from Aug 28, 2020

Conversation

danielsdeleo
Copy link
Contributor

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

Our compiled golang protos are not usable outside of automate because all of automate is one golang module with broken dependencies (dex won't install for some reason related to go modules). Fixing the dependency would still leave a situation where external consumers of the API would need to install all of Automate's deps just to get some structs. Making the external API a separate module will result in much better behavior.

In order for this approach to deliver the desired benefit, the external API must have no dependencies on the rest of Automate. You can run git grep -h -o 'chef/automate\S\+' | grep -v chef/automate/api | sort | uniq from the api/external directory to find the golang imports we need to fix. Prior to this PR, we get the following:

chef/automate/api/interservice/data_lifecycle
chef/automate/components/automate-gateway/api/iam/v2/policy
chef/automate/components/automate-grpc/protoc-gen-policy/iam
chef/automate/lib/errorutils

In this PR, I moved some message definitions from the inter service data lifecycle proto to the external API proto. This is a breaking change at the generated code level. In the go code for example, the structs are now in a different package. That said, the code as it is prior to this PR is "pre-broken" as it uses code with no compatibility guarantees (api/interservice protos) inside code that is supposed to have compatibility guarantees (api/external). Compatibility should be maintained at the protobuf wire level and the protobuf-as-JSON level, which, combined with the current brokenness of using our compiled golang protos, makes it unlikely we will break any customers.

⛓️ Related Resources

#4273 #4277

πŸ‘ Definition of Done

Code should work the same, except for the compatibility caveat described above. If you run it grep -h -o 'chef/automate\S\+' -- api/external | grep -v chef/automate/api/external | sort | uniq from the repo root, you no longer see the data lifecycle interservice protos in the output.

πŸ‘Ÿ How to Build and Test the Change

βœ… Checklist

πŸ“· Screenshots, if applicable

Signed-off-by: Daniel DeLeo <dan@chef.io>
@netlify
Copy link

netlify bot commented Aug 27, 2020

Deploy preview for chef-automate ready!

Built with commit 534b7a1

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

@stevendanna
Copy link
Contributor

dex won't install for some reason related to go modules

If the fundamental issue is still:

dexidp/dex#1578

Then we might consider the pinning strategy outlined in the comments there rather than our replace directive. I'm not sure if that would resolve the dependency issue or not though.

@srenatus
Copy link
Contributor

Was that not resolved with dexidp/dex#1750? I haven't kept close track, but I thought so... πŸ€”

@stevendanna
Copy link
Contributor

@srenatus Aha! I missed that. At the very least it is worth trying to get rid of our replace directive

@danielsdeleo danielsdeleo merged commit e206782 into master Aug 28, 2020
@danielsdeleo danielsdeleo deleted the dan/data-lifecyle-external branch August 28, 2020 19:04
satellite15 pushed a commit that referenced this pull request Sep 3, 2020
Signed-off-by: Daniel DeLeo <dan@chef.io>
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

3 participants