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

feat: add guardian proto #17

Merged
merged 46 commits into from
Nov 10, 2021
Merged

feat: add guardian proto #17

merged 46 commits into from
Nov 10, 2021

Conversation

rahmatrhd
Copy link
Member

No description provided.

@rahmatrhd rahmatrhd requested a review from ravisuhag July 12, 2021 03:23
option java_outer_classname = "ServiceManager";

// These annotations are used when generating the OpenAPI file.
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {
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 start using OpenAPI v3?

Copy link
Member Author

Choose a reason for hiding this comment

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

grpc-ecosystem/grpc-gateway#2147 the support for v3 still under development

odpf/guardian/guardian.proto Show resolved Hide resolved
odpf/guardian/guardian.proto Outdated Show resolved Hide resolved
odpf/guardian/guardian.proto Outdated Show resolved Hide resolved
@rahmatrhd rahmatrhd marked this pull request as ready for review July 15, 2021 03:33
@ravisuhag
Copy link
Member

LGTM. @kushsharma Can you also cross-check for any conventions we are following in proton?


// Provider contains information about external data provider such as BigQuery, Metabase, etc., credentials, policy, and allowed roles
message Provider {
uint32 id = 1;
Copy link
Member

Choose a reason for hiding this comment

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

I see we are using int ids everywhere, are these DB auto-increment ids? Why not use UUID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes DB auto-increment, no specific reason why we're not using UUID, just following traditional ways for creating entity. What would make it better to use UUID?

Copy link
Member

Choose a reason for hiding this comment

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

  • can't enumerate values
  • can't infer how many values are present in each table
  • don't have to talk to the database to construct a value, useful in certain ORM situations
  • perfectly indexable by most dbs
  • easier to shard when we need to distribute data into multiple dbs
  • unique across multiple tables/databases/space-time continuum
  • costly on size but storage is cheap

odpf/guardian/guardian.proto Outdated Show resolved Hide resolved
@kushsharma
Copy link
Member

@ravisuhag are we not versioning our REST APIs? Not versioning protobufs is fine I think as they get imported and get versioned while importing but REST APIs have no convention of knowing if something has breaking changes right?

@ravisuhag
Copy link
Member

+1 to versioning. Shall we right away start with v1 as folder as well and move protos to that? or any other better idea?

@rahmatrhd
Copy link
Member Author

rahmatrhd commented Jul 15, 2021

agree on adding v1 as folder inside existing api folder as well.

should we add the prefix /api as well in the url? => /api/v1/resources... currently we don't

@ravisuhag
Copy link
Member

let's skip /api if needed service should handle it.

@kushsharma
Copy link
Member

Yeah, we can version protobufs at directory/package level as well although once we have stable services, function definition hardly changes, only message fields do and that can be handled by only introducing new fields with new indices and supporting old fields as well. But yeah I am onboard with directory level versioning as well.
I need to refactor optimus protobufs though, went with the simplest one when started and now it has so many things going on.
Like this

.
└── proto
    └── foo
        └── bar
            ├── bat
            │   └── v1
            │       └── bat.proto // package foo.bar.bat.v1
            └── baz
                └── v1
                    ├── baz.proto         // package foo.bar.baz.v1
                    └── baz_service.proto // package foo.bar.baz.v1

@ravisuhag
Copy link
Member

@rahmatrhd lets make the discussed changes and then we can merge.

@rahmatrhd rahmatrhd changed the title add guardian proto feat: add guardian proto Aug 5, 2021
@ravisuhag
Copy link
Member

ravisuhag commented Aug 9, 2021

@rahmatrhd @kushsharma Are we moving protos to V1 folder?

@rohilsurana
Copy link
Member

package odpf.guardian; => package odpf.guardian.v1;
option go_package = "github.com/odpf/proton/guardian"; => option go_package = "github.com/odpf/proton/guardian/v1;guardianv1";

same goes for the directory as well.

@rohilsurana
Copy link
Member

Let's follow semver for openapiv2_swagger.info.version.

rahmatrhd and others added 23 commits September 23, 2021 09:25
feat: add details to each resource of an appeal
… the provider (#46)

* feat: added soft delete of resource feature

* fix: use is_deleted as a query string

Co-authored-by: Ishan Arya <ishanarya0@gmail.com>

Co-authored-by: Rahmat Hidayat <rahmatramahidayat@gmail.com>
@ravisuhag ravisuhag merged commit 9a0f069 into main Nov 10, 2021
@ravisuhag ravisuhag deleted the guardian branch November 10, 2021 04:39
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.

5 participants