feat: Allow yaml.SequenceNode type #1208
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1208 +/- ##
==========================================
+ Coverage 63.05% 63.34% +0.29%
==========================================
Files 23 23
Lines 3140 3165 +25
==========================================
+ Hits 1980 2005 +25
Misses 1050 1050
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AntonShadrinNN
requested review from
chengfang,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
|
I decided to use the abcd[0] syntax, as it is simpler in terms of parsing and more intuitive in terms of appearance. |
docs/configuration/images.md
Outdated
| argocd-image-updater.argoproj.io/fooalias.helm.image-tag: foo[1].image | ||
| ``` | ||
|
|
||
| This works also for both annotation `<image_alias>.helm.image-name` and annotation `<image_alias>.helm.image-tag`. |
There was a problem hiding this comment.
earlier in this doc, it says all 3 annotations will support this index notation: image-name, image-tag, image-spec. So the above line should be updated to include all 3.
docs/configuration/images.md
Outdated
| *Solution:* Use the index in square brackets of the item that needs to be updated, i.e. | ||
|
|
||
| ```yaml | ||
| argocd-image-updater.argoproj.io/fooalias.helm.image-tag: foo[1].image |
There was a problem hiding this comment.
to make it easier to understand which list element is targetted, you may want to change to use image foo-1 and foo-2 (not using another bar* image), and use foo-alias and foo[1] to refer to the 2nd element.
There was a problem hiding this comment.
Done, if I got the gist right.
pkg/argocd/update.go
Outdated
| idStr := matches[2] | ||
| id, err := strconv.Atoi(idStr) | ||
| if err != nil { | ||
| return fmt.Errorf("id \"%s\" in yaml array must match pattern ^(\\D+)\\[(\\d+)\\]$", idStr) |
There was a problem hiding this comment.
the declared pattern should be used in the error msg, to be consistent.
There was a problem hiding this comment.
My bad. Forgot to change it.
There was a problem hiding this comment.
Changed, including tests
pkg/argocd/update.go
Outdated
| keys := strings.Split(key, ".") | ||
|
|
||
| // any-string[1] | ||
| pattern := `^(.*)\[(.*)\]$` |
There was a problem hiding this comment.
we may want to use a more specific pattern to avoid invalid values like foo[x]:
pattern := `^([^[]+)\[(\d+)\]$`There was a problem hiding this comment.
The original version looked like this, but it makes it harder to return a clear error like this . The problem is that the search is based on the key. If it doesn't fit the pattern, then the search will follow it entirely, which means it will fall into the else block and simply write this value. Should I take a different approach, such as checking the validity of the strings and parsing them separately?
There was a problem hiding this comment.
It's a small difference, in that case, let's just keep what you have now.
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
chengfang
force-pushed
the
ashadrin/allow_yaml_sequence_node_type
branch
from
0680f20 to
c434ba0
Compare
Signed-off-by: Anton Shadrin <[email protected]>
pkg/argocd/update.go
Outdated
|
|
||
| // any-string[1] | ||
| pattern := `^(.*)\[(.*)\]$` | ||
| re := regexp.MustCompile(pattern) |
There was a problem hiding this comment.
How about moving the regexp compilation to the pkg level var? This func is called multiple times per image update and we should avoid recompiling the regexp repeatedly.
There was a problem hiding this comment.
That's a good point. Fixed it.
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]>
|
@chengfang First of all, thank you for your help. Can I ask you to tag these changes? I would like to use it to solve my original problem. |
Signed-off-by: Anton Shadrin <[email protected]> Co-authored-by: Anton Shadrin <[email protected]>
Signed-off-by: Anton Shadrin <[email protected]> Co-authored-by: Anton Shadrin <[email protected]> (cherry picked from commit e816c49) Signed-off-by: Denis Karpelevich <[email protected]>
3 participants
Closes #1206
Closes #1032
Fixes a situation where it was impossible to update images that were in the yaml array.
The code can probably be improved, I will be glad to follow the recommendations.