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

Add missing Google.Rpc types #26

Merged
merged 6 commits into from
Aug 20, 2024

Conversation

mattpolzin
Copy link
Contributor

This PR generates and copies some Google Rpc types into the Authzed library in response to a Dialyzer error that I saw once I started using the relatively new Bulk Permissions Check API.

lib/api/v1/permission_service.pb.ex:308:unknown_type
Unknown type: Google.Rpc.Status.t/0.

I admit that I am not confident we really want this type in this package, and also that it feels like some tooling along the way maybe dropped the ball on this one because the Authzed buf.yaml states it depends on googleapis which is where this type comes from, so I would think that part of buf's job would be to generate code for that dependency.

Nonetheless, perhaps this is a reasonable short term solution since it doesn't cram too much more into this library and it addresses a bug that stops you from using otherwise useful Authzed APIs.

I am happy to revert the commit that simply reorders a bunch of lines of code; that change was made automatically on my machine, but it's clearly not substantive.

@mattpolzin
Copy link
Contributor Author

@goodhamgupta looks like the Buf CI action wasn't new enough to read v2 configs. The latest version of the CI Action that replaced buf-setup-action should hopefully be happy.

@mattpolzin
Copy link
Contributor Author

Ah, they made buf-action do a bunch of stuff by default that needs to be turned off for this use case. Kind of annoying. Sorry for the back-and-forth on this, I can't test the CI out at work because our org disallows third-party GitHub Actions.

@goodhamgupta
Copy link
Owner

Hi @mattpolzin , thanks for making these changes! There are a few things we need to address:

  • The current CI is failing because Docker Compose is not installed on the image being used by the runner.
  • We also have a versioning issue: I originally intended to release the first version of this package as 1.0.0, but ended up updating the Buf schemas version and released updates as "patches" instead of "minor" versions. Given that the PR in its current state doesn't make changes to the existing schema, we have a few options:
    • We could deploy these changes as 0.0.13. This would go against what has been done in the repo so far, but it is the right approach since these changes are mostly related to fixing typing errors.
    • We could first deploy the existing version as 1.0.0, marking it as the first stable version release. Then, we could merge this PR and mark it as 1.0.1 or something similar.
    • We could deploy these changes to the existing version 0.0.12 and re-publish the package on Hex. However, this would cause all users running the current version of the package to receive the new changes. Personally, I want to avoid this using this method.

I apologize for the incorrect versioning strategy so far (completely my mistake), and I would like to get some feedback before making a change. I would also appreciate any other suggestions for deploying these changes.

@mattpolzin
Copy link
Contributor Author

mattpolzin commented Aug 16, 2024

The current CI is failing because Docker Compose is not installed on the image being used by the runner.

I'll fix this in the next few (work) days.

We also have a versioning issue [...] I apologize for the incorrect versioning

Hey, no worries in my opinion. This is your rodeo and I don't think there are too many bad options, though I agree with you about avoiding republishing the same version. If you feel like stabilizing the package where it's at now, publishing 1.0 before merging this sounds great to me, albeit a bit of extra work on your part. I also don't see any serious problems with your first option, though. As far as I know pre-1.0 releases are definitionally numbered at the discretion of the package author with no specific rules.

@goodhamgupta
Copy link
Owner

Thank you! Since there are atleast two organisations using this package in production, I'll first release the package in it's current state as 1.0.0, and then we can update the package version in this PR and merge it in.

@goodhamgupta
Copy link
Owner

Note that the CI is only failing because we were using the old syntax for compose i.e docker-compose instead of docker compose. Once you make this change the bump up the version this PR is good to go!

@goodhamgupta goodhamgupta merged commit db8ebc6 into goodhamgupta:main Aug 20, 2024
2 checks passed
@goodhamgupta goodhamgupta self-requested a review August 20, 2024 08:14
Copy link
Owner

@goodhamgupta goodhamgupta left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thanks for your work! 🙌🏽

@mattpolzin mattpolzin deleted the add-missing-rpc-types branch August 20, 2024 12:27
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.

2 participants