abcxyz / jvs

Apache License 2.0
8 stars 0 forks source link

chore: refactored service code into service/ folder in preparation for adding client code #33

Closed raserva closed 2 years ago

yolocs commented 2 years ago

I'm more supportive of writing the client in lumberjack since it'll be the primary user (of the client), and it already has multi-language setup.

@mikehelmick @sethvargo WDYT?

raserva commented 2 years ago

I'm more supportive of writing the client in lumberjack since it'll be the primary user (of the client), and it already has multi-language setup.

@mikehelmick @sethvargo WDYT?

Capturing some of the things i mentioned to Chen, as the reasons i want this here for now.

  1. Lumberjack will be our primary use case, but another (and the first) use case is for integration tests. It would be odd to have to have to pull in lumberjack in order to run our integration tests here.
  2. The clients main jobs will be pretty generic (pull down & cache primary key, then validate JWT) a more generic package such as abcxyz/pkg might be a better eventual home.
  3. Having access to jvs objects, specifically for deserializing custom claims could be useful when we have more complex use cases, such as if we want to have complex scope (for the service to see if the token is applicable to it)
yolocs commented 2 years ago

Lumberjack will be our primary use case, but another (and the first) use case is for integration tests. It would be odd to have to have to pull in lumberjack in order to run our integration tests here.

IIUC, the integration test will only be written in Go. Do we need Java here as well?

The clients main jobs will be pretty generic (pull down & cache primary key, then validate JWT) a more generic package such as abcxyz/pkg might be a better eventual home.

Agreed, for Go. We need to rethink whether we want to have a "pkg" for Java. My immediate answer is no and would prefer putting it where it's actually used (in this case lumberjack).

Having access to jvs objects, specifically for deserializing custom claims could be useful when we have more complex use cases

If it's something we need in the next 10 PRs, I'd be supportive. Otherwise we could defer it.

The main benefit of having the code in lumberjack is that it's the only place where the logic will be used (other than integ test). And we can save the trouble of restructuring all the code here and leave this repo to be a pure Go repo.

My proposal is:

  1. Add Go logic in pkg
  2. Use pkg in jvs integ test and in lumberjack
  3. Implement the java logic in lumberjack java client (it's a lib anyway)
  4. (Future) If there is another need to use the lib somewhere else than lumberjack, move the java code to a better home.
sethvargo commented 2 years ago

I think I'd rather keep the clients separate.

mikehelmick commented 2 years ago

I think the JVS client should live in the JVS repo (and same for any other components).

I'm -1 on the current refactor though, as I think the service itself should be at the root of the repo and just the clients in a sub-directory.

yolocs commented 2 years ago

To be clear, I think the idea is to have clients in multiple languages. To me it seems good to limit such multi-language surface and that's why I suggested lumberjack.

RE folder structure in this PR - I don't recall we can have go module (the future go client) under a go module (the current go module)?

sethvargo commented 2 years ago

I think I'd rather keep the clients separate.

To be clearer, I think each project should vend its own client libraries in the same repository as the project.

yolocs commented 2 years ago

I don't recall we can have go module (the future go client) under a go module (the current go module)?

I was wrong. There can't be a sub-module in a module but there could be a module under another module's folder.

I think each project should vend its own client libraries in the same repository as the project.

We can establish this as a convention and make it the future home of generated gRPC clients as well.

raserva commented 2 years ago

Alright, the consensus seems to be to put the clients in this project in a sub-folder, and leave the service code at root level. I'll close this PR for now.