Add a piped component that watches app configs #2772
Conversation
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.
|
Code coverage for golang is
|
| func (r *Reporter) checkApps(ctx context.Context) (err error) { | ||
| if len(r.gitRepos) == 0 { | ||
| r.logger.Info("no repositories were configured for this piped") | ||
| return | ||
| } | ||
|
|
||
| var ( | ||
| unusedApps = make([]*pipedservice.ApplicationConfiguration, 0) | ||
| appsToBeUpdated = make([]*pipedservice.ApplicationConfiguration, 0) | ||
| appsMap = r.listApplications() | ||
| ) | ||
| for repoID, repo := range r.gitRepos { |
There was a problem hiding this comment.
We can break this function to be simpler ones. For example
func scan(cox context.Context) error
for _, repo := range r.gitRepos {
// 1. Fetch to update the local data of each repo to ensure they are up-to-date.
// 2. Continue if there was no newly added commits.
// 3. shouldScanRepos = append(shouldScanRepos, repo)
}
for _, repo := range shouldScanRepos {
if err := scanRepo(cox context.Context, repo, previousHash, currentHash); err == nil {
// mark the new hash to the map
}
}
}
func scanRepo(ctx context.Context, repo Repo, previousHash, currentHash string) error {
...
}There was a problem hiding this comment.
Obviously more readable than before
There was a problem hiding this comment.
But to be honest, since we need to send all unregisteredApps each time, we have to scan all repositories anyway.
There was a problem hiding this comment.
Piped sends all the latest configs every time whatever the repo is changed or not. The reason is the cache in control-plane is likely to be removed, and configs in Git is also likely to be removed
As mentioned in the PR description, I'm looking to letting Piped send all the latest configs every time whatever the repo is changed or not. The reasons are:
- Even if there are no new commits to the repository, but the Control-plane cache may have already been removed.
- If you send only the configuration to be updated, you can't address the case that the file itself is deleted from the repo.
What do you think about it?
There was a problem hiding this comment.
I get your points.
Checking by the diff still be valid in those cases. Scanning the full repository can be done for only the first time and then just the diff from the second time. Of course, we should send the full list of "un-registered" ones and the changed "registered" ones to the control-plane.
About the deleted files I think we can ignore them since app deletion only be allowed from web.
There was a problem hiding this comment.
Ah, you mean to separate updating the repository from reading the files. Cool, I have no objection.
About the deleted files I think we can ignore them since app deletion only be allowed from web.
Sorry for confusing you. I was afraid that even though the files of unregistered applications were deleted, they were still in the cache and would be suggested when registering an application.
There was a problem hiding this comment.
I was afraid that even though the files of unregistered applications were deleted, they were still in the cache and would be suggested when registering an application.
After having the list of changed files, we still have to read their content to get app_name, kind... So we can know that case at parsing time to handle as normal.
There was a problem hiding this comment.
Sorry, my words were possibly not enough. I was talking about unregistered applications.
Could you tell me about your thoughts on always sending ALL unregistered apps each time?
There was a problem hiding this comment.
Could you tell me about your thoughts on always sending ALL unregistered apps each time?
Yes, I think that is ok, we have to send the full list of unregistered apps because they are saved in the cache.
We are on the same page about that topic.
| for _, app := range apps { | ||
| gitPath := app.GetGitPath() | ||
| _ = filepath.Join(repo.GetPath(), gitPath.Path, gitPath.ConfigFilename) | ||
| // TODO: Collect applications that need to be updated | ||
| } |
There was a problem hiding this comment.
Instead of iterate apps to check like this, we can use git diff previousHash currentHash --name-only to have a list of changed files. And then find the updated ones and un-registered ones based on that list.
There was a problem hiding this comment.
I was trying to parse and inspect the application configs was changed to avoid unneeded communication. But yours seems to be enough.
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.
|
Code coverage for golang is
|
| func (r *Reporter) checkApps(ctx context.Context) (err error) { | ||
| if len(r.gitRepos) == 0 { | ||
| r.logger.Info("no repositories were configured for this piped") | ||
| return | ||
| } | ||
|
|
||
| var ( | ||
| unusedApps = make([]*pipedservice.ApplicationConfiguration, 0) | ||
| appsToBeUpdated = make([]*pipedservice.ApplicationConfiguration, 0) | ||
| appsMap = r.listApplications() | ||
| ) | ||
| for repoID, repo := range r.gitRepos { |
There was a problem hiding this comment.
I get your points.
Checking by the diff still be valid in those cases. Scanning the full repository can be done for only the first time and then just the diff from the second time. Of course, we should send the full list of "un-registered" ones and the changed "registered" ones to the control-plane.
About the deleted files I think we can ignore them since app deletion only be allowed from web.
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.
|
Code coverage for golang is
|
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.
pkg/model/common.go
Outdated
| } | ||
|
|
||
| // BuildGitPathID builds a unique path between repositories. | ||
| func BuildGitPathID(repoID, path, configFileName string) string { |
There was a problem hiding this comment.
I just want to look into if the application is registered, using GitPath. I'm still wondering if we can do that in other ways.
|
Code coverage for golang is
|
|
@nghialv I am very grateful for your careful confirmation. Based on your suggestion, I replaced the part that used to be the TODO comment with the implementation that actually updates it. It's not exactly what you suggested, but I would say it's more readable than before. Please take a look at it when you have time. |
Sure. Let me check it. |
pkg/config/deployment.go
Outdated
|
|
||
| type GenericDeploymentSpec struct { | ||
| Name string `json:"name"` | ||
| EnvID string `json:"env_id"` |
There was a problem hiding this comment.
| EnvID string `json:"env_id"` | |
| EnvID string `json:"envId"` |
There was a problem hiding this comment.
Btw, isn't the name better than the id?
There was a problem hiding this comment.
I was worried about the uniqueness of env_name but let's leave it to the user to maintain the uniqueness.
It's obvious that the application name shouldn't be unique, but ideally the env name should be unique because Environment is a project-wide concept.
To do that with a document-oriented database, we need to prepare some kind of extra table with the name like envname to ensure uniqueness.
But we're planning to migrate Env to Labels in the future, again, for now let's leave it to the user to maintain the uniqueness.
There was a problem hiding this comment.
If we proceed with the Name as non-unique, we will have to add a not neat method to environmentstore. I'd like to hear your opinion on uniqueness before implementing it.
|
Nice. It became more readable. |
Co-authored-by: Le Van Nghia <[email protected]>
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.
|
Code coverage for golang is
|
| func shouldSkip(repoID, cfgFilePath string, registeredAppPaths map[string]struct{}, wantRegistered bool) bool { | ||
| if !strings.HasSuffix(cfgFilePath, model.DefaultDeploymentConfigFileExtension) { | ||
| return true | ||
| } | ||
| gitPathKey := makeGitPathKey(repoID, cfgFilePath) | ||
| if _, registered := registeredAppPaths[gitPathKey]; registered != wantRegistered { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
How about removing this function and using its code directly?
Because handling for both "registered" and "unregistered" cases makes it harder to read.
There was a problem hiding this comment.
Right. It should not be made common.
| apps = append(apps, as...) | ||
| } | ||
| if len(apps) == 0 { | ||
| return nil |
There was a problem hiding this comment.
Do you think that we should send this empty list to the control plane to delete the cache?
There was a problem hiding this comment.
Or we can have a simple mark in this Reporter to avoid sending continuous this kind of request.
There was a problem hiding this comment.
To avoid sending meaningless requests simply, I applied this way. Cache in Control-plane gets periodically removed, so I thought it's fine. But it's worth considering.
|
@nghialv Revised! |
|
Code coverage for golang is
|
nghialv
left a comment
nghialv
left a comment
There was a problem hiding this comment.
Finished my review. Just some nits. 👍
| logger *zap.Logger | ||
|
|
||
| // Whether it already swept all unregistered apps from control-plane. | ||
| // Not goroutine safe. |
There was a problem hiding this comment.
Do we need this kind of comment since we are not spawning any new goroutines in this package?
There was a problem hiding this comment.
I just left these comments for when we need to spawn multiple goroutines for performance improvement in the future because file reading is embarrassingly parallel. But if you think about it, they aren't used for that reading part. It's completely YAGNI👍
| if !strings.HasSuffix(cfgRelPath, model.DefaultDeploymentConfigFileExtension) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: We can move this to before line 305 and use path to check the extension. It will reduce a lot of unnecessary filepath.Rel calls.
|
@nghialv Fixed |
|
Code coverage for golang is
|
|
Looks good! Go ahead. |
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Update the given application configurationsThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.2. Make the unused application configurations cache up-to-dateThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.3. Implement environmentstore.GetByNameThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.4. Return an error if any one of required field of Application is emptyThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.5. Convert Kind string into dedicated typeThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.6. Consider changing the default application config nameThis was created by todo plugin since "TODO:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne.7. Think about sync interval of app config reporterThis was created by todo plugin since "FIXME:" was found in 5ec2cbb when #2772 was merged. cc: @nakabonne. |
|
I just added a TODO comment |
|
Code coverage for golang is
|
|
Way to go 🚀 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR adds a new Piped component that primarily watches two types of Applications that is:
For the first one (already registered):
For the second one (unregistered):
Once this gets merged, I will drill into the detailed implementation.
Which issue(s) this PR fixes:
Fixes #2755
Ref #2772
Ref #2750
Does this PR introduce a user-facing change?: