Ensure to load image watcher config #1251
Conversation
|
Code coverage for golang is
|
pkg/config/piped.go
Outdated
| repos[repo.RepoID] = struct{}{} | ||
|
|
||
| if len(repo.Includes) != 0 && len(repo.Excludes) != 0 { | ||
| return fmt.Errorf("both includes and excludes are given in %s", repo.RepoID) |
There was a problem hiding this comment.
Do we have any reasons for not allowing this?
There was a problem hiding this comment.
Our recognition might be different. I'm thinking Includes is like a whitelist, and Excludes is like a blacklist.
- When using Excludes, include all files that are not specified
- When using Includes, include all specified files
If both are given, we can't decide which to prioritize.
There was a problem hiding this comment.
I got it. These are lists of filenames right, not regexes.
IMO, we can ignore this error by taking the excludes with a higher priority.
There was a problem hiding this comment.
I'm unfamiliar with the standard, but is it general to prioritize the Excludes? I'm worried about letting users be confused.
There was a problem hiding this comment.
I think there are no rules for that.
(or just a ref: https://gist.github.com/ento/cf5593d4fac3f72d3765b3fe2ed2a204)
But I think in our case, by missing/wrong-configuration, no deployment was triggered is better/safer than a deployment was triggered.
There was a problem hiding this comment.
I agree with that, okay, let's make Excludes top priority and will tell about the priority to users well.
|
Code coverage for golang is
|
| dir := filepath.Join(repoRoot, SharedConfigurationDirName) | ||
| files, err := ioutil.ReadDir(dir) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("failed to read %s: %w", dir, err) |
There was a problem hiding this comment.
I think no .pipe directory is a normal case. In that case, is piped sending this error to the log system?
There was a problem hiding this comment.
Exactly, it should be handled as not found case.
There was a problem hiding this comment.
Maybe we should compare with os.IsNotExist.
| if _, ok := whiteList[f.Name()]; ok { | ||
| filtered = append(filtered, f) | ||
| } | ||
| } |
There was a problem hiding this comment.
I expected it will be pointed out by you lol
It's tough to go out into it in English, but I'd say these processes are quite similar, so using if-else makes it readble. But it depends on the reader. What do you think?
There was a problem hiding this comment.
😄
I always follow return-early and avoid-deep-nesting.
Because when we see a return we don't need to care about all of the later parts.
Using if-else we don't have to care about the else's inside but still have to check the part after that else.
There was a problem hiding this comment.
Gotcha 😄
No objection with that philosophy!
|
@nghialv Fixed them! |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
@nghialv Fixed done. |
|
/approve |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR allows piped to load image watcher files after filtering those based on includes and excludes.
Which issue(s) this PR fixes:
Fixes #1211
Does this PR introduce a user-facing change?: