Add RPCs to list unregistered apps #2847
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "sort" | ||
| "strings" | ||
| "time" | ||
|
|
||
|
|
@@ -66,6 +67,7 @@ type WebAPI struct { | |
| pipedProjectCache cache.Cache | ||
| envProjectCache cache.Cache | ||
| insightCache cache.Cache | ||
| redis redis.Redis | ||
|
|
||
| projectsInConfig map[string]config.ControlPlaneProject | ||
| logger *zap.Logger | ||
|
|
@@ -102,6 +104,7 @@ func NewWebAPI( | |
| pipedProjectCache: memorycache.NewTTLCache(ctx, 24*time.Hour, 3*time.Hour), | ||
| envProjectCache: memorycache.NewTTLCache(ctx, 24*time.Hour, 3*time.Hour), | ||
| insightCache: rediscache.NewTTLCache(rd, 3*time.Hour), | ||
| redis: rd, | ||
| logger: logger.Named("web-api"), | ||
| } | ||
| return a | ||
|
|
@@ -599,6 +602,85 @@ func (a *WebAPI) validatePipedBelongsToProject(ctx context.Context, pipedID, pro | |
| return nil | ||
| } | ||
|
|
||
| func (a *WebAPI) ListUnregisteredApplications(ctx context.Context, _ *webservice.ListUnregisteredApplicationsRequest) (*webservice.ListUnregisteredApplicationsResponse, error) { | ||
| claims, err := rpcauth.ExtractClaims(ctx) | ||
| if err != nil { | ||
| a.logger.Error("failed to authenticate the current user", zap.Error(err)) | ||
| return nil, err | ||
| } | ||
|
|
||
| // Collect all apps that belong to the project. | ||
| key := makeUnregisteredAppsCacheKey(claims.Role.ProjectId) | ||
| c := rediscache.NewHashCache(a.redis, key) | ||
| // pipedToApps assumes to be a map["piped-id"][]*model.ApplicationInfo | ||
|
Member
There was a problem hiding this comment. The returned data will not be decoded to this
Member
Author
There was a problem hiding this comment. Seriously? I am sorry, I had neglected to do my research. I assumed that |
||
| pipedToApps, err := c.GetAll() | ||
| if errors.Is(err, cache.ErrNotFound) { | ||
| return &webservice.ListUnregisteredApplicationsResponse{}, nil | ||
| } | ||
| if err != nil { | ||
| a.logger.Error("failed to get unregistered apps", zap.Error(err)) | ||
| return nil, status.Error(codes.Internal, "Failed to get unregistered apps") | ||
| } | ||
|
|
||
| // Integrate all apps cached for each Piped. | ||
| allApps := make([]*model.ApplicationInfo, 0) | ||
| for _, as := range pipedToApps { | ||
| apps, ok := as.([]*model.ApplicationInfo) | ||
| if !ok { | ||
| return nil, status.Error(codes.Internal, "Unexpected data cached") | ||
| } | ||
| allApps = append(allApps, apps...) | ||
| } | ||
|
|
||
| return &webservice.ListUnregisteredApplicationsResponse{ | ||
| Repos: groupAppsByRepo(allApps), | ||
| }, nil | ||
| } | ||
|
|
||
| func groupAppsByRepo(apps []*model.ApplicationInfo) []*webservice.ListUnregisteredApplicationsResponse_Repo { | ||
| if len(apps) == 0 { | ||
| return []*webservice.ListUnregisteredApplicationsResponse_Repo{} | ||
| } | ||
| if len(apps) == 1 { | ||
| return []*webservice.ListUnregisteredApplicationsResponse_Repo{ | ||
| {Id: apps[0].RepoId, Apps: apps}, | ||
| } | ||
| } | ||
|
|
||
| // Make a map from repo-id to apps. | ||
| repoToApps := make(map[string][]*model.ApplicationInfo) | ||
| for _, app := range apps { | ||
| if _, ok := repoToApps[app.RepoId]; !ok { | ||
| repoToApps[app.RepoId] = []*model.ApplicationInfo{} | ||
| } | ||
| repoToApps[app.RepoId] = append(repoToApps[app.RepoId], app) | ||
| } | ||
|
|
||
| // Tidy apps. | ||
| repos := make([]*webservice.ListUnregisteredApplicationsResponse_Repo, 0, len(repoToApps)) | ||
| for repoID, as := range repoToApps { | ||
| // Eliminate duplicated apps | ||
| tidiedApps := make([]*model.ApplicationInfo, 0, len(as)) | ||
| gitPaths := make(map[string]struct{}) | ||
| for _, app := range as { | ||
| if _, ok := gitPaths[app.GetPath()]; ok { | ||
| continue | ||
| } | ||
| gitPaths[app.GetPath()] = struct{}{} | ||
| tidiedApps = append(tidiedApps, app) | ||
| } | ||
|
|
||
| sort.Slice(tidiedApps, func(i, j int) bool { | ||
| return tidiedApps[i].GetPath() < tidiedApps[j].GetPath() | ||
| }) | ||
| repos = append(repos, &webservice.ListUnregisteredApplicationsResponse_Repo{ | ||
| Id: repoID, | ||
| Apps: tidiedApps, | ||
| }) | ||
| } | ||
| return repos | ||
|
Member
There was a problem hiding this comment. There are some concerns around the way we are grouping applications by repo-id and eliminating duplicates by GitPath. Since we don't have any guide or restriction on the repository ID across Piped currently, so users can use the same repo ID in different Pipeds for different repositories. For example:
That will result in a not-correct list. So I think we can return the raw data from the cache that includes PipedID in the response.
Member
Author
There was a problem hiding this comment. For sure. Obviously repo-id isn't a project-wide concept. Apparently It's enough to give back directly grouped by Piped.
Member
Author
There was a problem hiding this comment. The response structure depends on how to show the list on the adding form. Of course it will be able to change, but tell me a little bit more about your thoughts. I'm guessing the unregistered list on the web is going to be kind of like:
Therefore, the response of ListUnregisteredApplications would be like: message ListUnregisteredApplicationsResponse {
message Piped {
string id = 1;
repeated model.ApplicationInfo apps = 2;
}
repeated Piped pipeds = 1;
}But if it's enough to show a flat list like:
just add a field called PipedId to ApplicationInfo and return a list of ApplicationInfo. |
||
| } | ||
|
|
||
| // TODO: Validate the specified piped to ensure that it belongs to the specified environment. | ||
| func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicationRequest) (*webservice.AddApplicationResponse, error) { | ||
| claims, err := rpcauth.ExtractClaims(ctx) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the
redisexplicitly here or just passpipedCfgCacheas we do forpipedStatCache? 🤔Since, for instance, we may want to support not just redis but memcached as shared cache in the future, should keep all of those specified things out of our logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel you. Honestly I also want to abstract it in such a way. But we want to generate HashCache with different keys for each project in the method, that's why PipedAPI needs to have a connection to the redis server.
https://github.com/pipe-cd/pipe/pull/2847/files#diff-dc8d28c3b055fcad0f022406b744ce756e3be8c630b6c40aef12e026c6a58dcfR1014
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay I see, get the point here, thanks 👍
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just created this issue for those things, lets think about it later 👍
#2865
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍