Add an rpc to update registered apps that have been changed #2830
Conversation
|
Code coverage for golang is
|
pkg/app/api/grpcapi/piped_api.go
Outdated
| opts := datastore.ListOptions{ | ||
| // NOTE: Assume that no more than one application is referring to a single configuration file. | ||
| Limit: 1, | ||
| Filters: []datastore.ListFilter{ | ||
| { | ||
| Field: "ProjectId", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: projectID, | ||
| }, | ||
| { | ||
| Field: "PipedId", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: pipedID, | ||
| }, | ||
| { | ||
| Field: "GitPath.Repo.Id", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: appInfo.RepoId, | ||
| }, | ||
| { | ||
| Field: "GitPath.Path", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: appInfo.Path, | ||
| }, | ||
| { | ||
| Field: "GitPath.ConfigFilename", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: appInfo.ConfigFilename, | ||
| }, | ||
| { | ||
| Field: "Disabled", | ||
| Operator: datastore.OperatorEqual, | ||
| Value: false, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Application ID can be included in the request by Piped to avoid this listing.
There was a problem hiding this comment.
Do you mean to uniquely identify the application based on the RepoID, Path, and ConfigFilename, get the id, and cache it on the Piped side?
There was a problem hiding this comment.
No. I think since this is for updating the existing applications then Piped already knew their ID before sending this UpdateApplicationConfigurations request.
https://github.com/pipe-cd/pipe/blob/master/pkg/app/piped/appconfigreporter/appconfigreporter.go#L146
https://github.com/pipe-cd/pipe/blob/master/pkg/app/piped/appconfigreporter/appconfigreporter.go#L197
So I meant that we can add an application_id field to this https://github.com/pipe-cd/pipe/blob/master/pkg/model/common.proto#L59-L65 model or the RPC request to avoid re-finding each one on the server to update.
There was a problem hiding this comment.
More specifically, we can update this map to be a map from GitPathKey to AppID (make(map[string]string, len(apps)))
There was a problem hiding this comment.
Sounds nice. I originally applied in this way this because I was trying to allow multiple applications to refer to a single config file, but now that the premise has changed, it seems better to identify them by ID.
|
@nghialv I reimplemented based on your suggestion! |
|
Code coverage for golang is
|
| _, _, _, err := rpcauth.ExtractPipedToken(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
since we have appInfo.Id, better to validate if the updating application belongs to the requested piped as other RPCs.
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Consider bulk-updating multiple appsThis was created by todo plugin since "TODO:" was found in d22b018 when #2830 was merged. cc: @nakabonne. |
|
Code coverage for golang is
|
|
/lgtm |
| // Scan all files because it seems to be the first commit since Piped starts. | ||
| apps := make([]*model.ApplicationInfo, 0) | ||
| err := fs.WalkDir(r.fileSystem, repo.GetPath(), func(path string, d fs.DirEntry, err error) error { | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Just want to ask, do we have any reason to not use that scanAllFiles function?
There was a problem hiding this comment.
@nghialv I feel you. Actually we can use the original scanAllFiles() removed just now. But we have to retrieve appID from registeredAppPaths only if called by findRegisteredApps. It is not a shared helper function anymore. It was not too huge function and originally it has been received shouldSkip which makes us confused.
So I settled on writing a bit overlapping logic directly into each function.
|
/lgtm |
|
Nice. |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR adds an RPC to update registered enabled apps.
As discussed previously, it is based on the assumption that a single app config file will not be referenced by multiple enabled apps. This should be made explicit in the documentation. But well eventually this will be able to be validated when added.
One case where is likely happen is enabling a disabled app that refers the same app config file. Possibly this can be addressed by the validation.
Which issue(s) this PR fixes:
Fixes #2803
Does this PR introduce a user-facing change?: