Define unregistered app cache #3313
Conversation
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
Code coverage for golang is |
|
@Hosshii should run |
|
Thank you! |
|
Code coverage for golang is |
| "github.com/pipe-cd/pipecd/pkg/redis" | ||
| ) | ||
|
|
||
| type Cache interface { |
There was a problem hiding this comment.
Nice added 👍 But I think there is no reason to explicitly mention that it's a cache. How about renaming this interface as Store just like other InsightStore or CommandStore 👀
There was a problem hiding this comment.
At first I was going to use store, but I thought there was a difference between store, which is persistent, and cache, which is not. And since redis is not persistent, I thought it would be better to use cache.
There was a problem hiding this comment.
I get your point! To me, something called Cache means that data in it can be fetched from another data source; in that case, this current data source is the only one we can use to get/store what we want. But you have a point too 😄 Happy to hear other opinions!
There was a problem hiding this comment.
I now understand your point of view. It is true that store seems more appropriate in that sense. Also, it might be better in terms of unity with the others.
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
| } | ||
| } | ||
|
|
||
| func (c *store) ListUnregisteredApplications(ctx context.Context, projectID string) ([]*model.ApplicationInfo, error) { |
There was a problem hiding this comment.
ctx is unused in ListUnregisteredApplications
|
Code coverage for golang is |
|
|
||
| if err != nil { | ||
| c.logger.Error("failed to get unregistered apps", zap.Error(err)) | ||
| return nil, status.Error(codes.Internal, "Failed to get unregistered apps") |
There was a problem hiding this comment.
gRPC error is a special error type used for gRPC communication so it should only be returned by gRPC services such as web-api, piped-api. All other internal packages return only the normal error then the callers (web-api, ...) have to convert them to gRPC error.
|
Code coverage for golang is |
|
I fixed. |
|
Good job. |
| ) | ||
|
|
||
| type Store interface { | ||
| ListUnregisteredApplications(ctx context.Context, projectID string) ([]*model.ApplicationInfo, error) |
There was a problem hiding this comment.
I think that ListApplications is better since this is under the directory of unregisterdappstore.
but it's up to you.
|
|
||
| type Store interface { | ||
| ListUnregisteredApplications(ctx context.Context, projectID string) ([]*model.ApplicationInfo, error) | ||
| PutUnregisteredApplications(projectID, pipedID string, apps []*model.ApplicationInfo) error |
|
Nice! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Define unregistered app cache to remove redis from dependency of pipd_api and web_api.
The implementation of cache is almost copy of
ListUnregisteredApplicationsin web_api andReportUnregisteredApplicationConfigurationsin piped_api.Which issue(s) this PR fixes:
rel #2865
Does this PR introduce a user-facing change?: