Ensure to watch image from image providers #1117
Conversation
|
Code coverage for golang is
|
nakabonne
force-pushed
the
image-watcher
branch
from
15c2474
to
c4ef830
Compare
nakabonne
force-pushed
the
image-watcher
branch
from
c4ef830
to
8995bb2
Compare
|
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
|
nakabonne
force-pushed
the
image-watcher
branch
from
8995bb2
to
177f903
Compare
|
Code coverage for golang is
|
nakabonne
force-pushed
the
image-watcher
branch
from
177f903
to
bff5808
Compare
|
Code coverage for golang is
|
nakabonne
force-pushed
the
image-watcher
branch
from
bff5808
to
fc2edcd
Compare
|
Code coverage for golang is
|
nakabonne
force-pushed
the
image-watcher
branch
from
fc2edcd
to
49576fe
Compare
|
Code coverage for golang is
|
nakabonne
force-pushed
the
image-watcher
branch
from
49576fe
to
23f7fb3
Compare
| "github.com/pipe-cd/pipe/pkg/model" | ||
| ) | ||
|
|
||
| // Provider acs as a container registry client. |
| Repo string | ||
| } | ||
|
|
||
| func (i ImageName) String() string { |
There was a problem hiding this comment.
return path.Join(i.Domain, i.Repo)
pkg/model/image_name.go
Outdated
| if i.Tag != "" { | ||
| tag = ":" + i.Tag | ||
| } | ||
| return fmt.Sprintf("%s%s", i.ImageName.String(), tag) |
There was a problem hiding this comment.
nit:
if i.Tag == "" {
return i.ImageName.String()
}
return fmt.Sprintf("%s:%s", i.ImageTag.String(), i.Tag)| Name string `json:"name"` | ||
| Type model.ImageProviderType `json:"type"` | ||
| // Default is five minute. | ||
| PullInterval Duration `json:"pullInterval"` | ||
|
|
||
| DockerhubConfig *ImageProviderDockerhubConfig | ||
| GCRConfig *ImageProviderGCRConfig | ||
| ECRConfig *ImageProviderECRConfig | ||
| } | ||
|
|
||
| type genericPipedImageProvider struct { |
There was a problem hiding this comment.
line 384: Dockerhub -> DockerHub
| switch cfg.Type { | ||
| case model.ImageProviderTypeGCR: | ||
| return gcr.NewProvider(cfg.Name, cfg.GCRConfig, doChallenge, logger) | ||
| case model.ImageProviderTypeDockerhub: |
pkg/config/piped.go
Outdated
| Type model.ImageProviderType | ||
| Name string `json:"name"` | ||
| Type model.ImageProviderType `json:"type"` | ||
| // Default is five minute. |
There was a problem hiding this comment.
nit: 5m (to be same with the specifying in the config file)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| _, err = repository.Tags(ctx).All(ctx) |
There was a problem hiding this comment.
As they described:
For repositories with a large number of tags, this response may be quite large. If such a response is expected, one should use the pagination.
https://docs.docker.com/registry/spec/api/#listing-image-tags
There was a problem hiding this comment.
Sorry for the lack of description, the implementation of GCR is still WIP, and we decided to prioritize ECR over GCR. So will just leave TODO comments.
|
|
||
| go w.run(ctx, p, cfg.PullInterval.Duration()) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This is exited immediately after starting goroutines for all image providers.
Please ensure that all of its child tasks must be stopped before exiting.
| } | ||
| includes = append(includes, target.Includes...) | ||
| excludes = append(excludes, target.Excludes...) | ||
| } |
There was a problem hiding this comment.
nit: Instead of finding and saving these include/exclude list, just finding and marking the target.
There was a problem hiding this comment.
Okay, let me address it when I work on this 🙏 :
https://github.com/pipe-cd/pipe/blob/1c4d30e9b310b4e180af234efcffe9e142ba4d7d/pkg/config/image_watcher.go#L31-L32
| return true | ||
| } | ||
| branch := repo.GetClonedBranch() | ||
| if err := repo.Pull(ctx, branch); err != nil { |
There was a problem hiding this comment.
git.Repo is a pointer and pointing to the same disk location of git data.
This Pull can be called from multiple goroutines and may cause a data race.
|
@nakabonne Reviewing is not finished yet, but please check the comments. |
|
@nghialv Fixed. Could you take a look? |
|
Code coverage for golang is
|
pkg/model/imageprovider.go
Outdated
| @@ -17,7 +17,7 @@ package model | |||
| type ImageProviderType string | |||
|
|
|||
| const ( | |||
| ImageProviderTypeDockerhub ImageProviderType = "DOCKERHUB" | |||
| ImageProviderTypeDockerHub ImageProviderType = "DOCKERHUB" | |||
| mu sync.Mutex | ||
|
|
||
| // Indexed by repo id. | ||
| gitRepos sync.Map |
There was a problem hiding this comment.
Not sure why do we need to use a sync map instead of a normal map?
There was a problem hiding this comment.
Oops, you're right. A map can be read when it's being read. Gonna fix them.
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Stop listing all tagsThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.2. Give back latest image from GCRThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.3. Use credentials for GCR configured by userThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.4. Handle the no challenge caseThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.5. Control not to reach the rate limitThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.6. Compares between image repos in the image registry and image repos in gitThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.7. Make it possible to push outdated images to GitThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne.8. Load image watcher configThis was created by todo plugin since "TODO:" was found in dc28c17 when #1117 was merged. cc: @nakabonne. |
|
@nghialv I really appreciate your polite review. Please take a look again 😄 |
|
Code coverage for golang is
|
nghialv
changed the title
|
Nice. |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
|
Thanks 😃 |
What this PR does / why we need it:
Base implementation of #1114.
Sorry for the huge patch. I'd be happy to work on tiny issues respectively after this.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: