Extend AWS ECR regex for c2s support #20648
Conversation
ethanchowell
changed the title
ethanchowell
force-pushed
the
echowell/aws-ecr
branch
from
2c02cc3 to
06571b7
Compare
|
Hi @ethanchowell , Thanks for your contributions! Best, |
|
I'm not aware of any official docs, but can point to the ecr-credential-helper that implements this pattern successfully. Slightly modified of course to account for existing tests within the project. |
|
@ethanchowell Hi, thanks for your contribution, please resolve the DCO failure in CI. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20648 +/- ##
===========================================
+ Coverage 45.36% 66.23% +20.86%
===========================================
Files 244 1045 +801
Lines 13333 113501 +100168
Branches 2719 2845 +126
===========================================
+ Hits 6049 75174 +69125
- Misses 6983 34214 +27231
- Partials 301 4113 +3812
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Ethan Howell <[email protected]>
Signed-off-by: Ethan Howell <[email protected]>
Signed-off-by: Ethan Howell <[email protected]>
Signed-off-by: Ethan Howell <[email protected]>
ethanchowell
force-pushed
the
echowell/aws-ecr
branch
from
43f2ba5 to
3bebf57
Compare
| @@ -67,7 +67,7 @@ func parseAccountRegion(url string) (string, string, error) { | |||
| if rs == nil { | |||
| return "", "", errors.New("bad aws url") | |||
| } | |||
| return rs[1], rs[2], nil | |||
| return rs[1], rs[3], nil | |||
There was a problem hiding this comment.
Please add length check for rs before this line.
There was a problem hiding this comment.
Happy to add it, but shouldn't that always evaluate to true based on the docs for FindStringSubmatch? In the event of no match it would just be nil.
Signed-off-by: Ethan Howell <[email protected]>
| @@ -64,10 +64,10 @@ func newAdapter(registry *model.Registry) (*adapter, error) { | |||
|
|
|||
| func parseAccountRegion(url string) (string, string, error) { | |||
| rs := ecrRegexp.FindStringSubmatch(url) | |||
| if rs == nil { | |||
| if rs == nil || len(rs) < 3 { | |||
There was a problem hiding this comment.
Since the following logic extract the rs[3], so it may be necessary to use < 4 here instead?
Signed-off-by: Ethan Howell <[email protected]>
|
@vincentni Hi, please help to review this PR as well if you have time, thanks. |
|
@Vad1mo Please help to review this PR as well, thanks. |
* Extend regex for c2s support Signed-off-by: Ethan Howell <[email protected]> * Add more test cases Signed-off-by: Ethan Howell <[email protected]> * Update region Signed-off-by: Ethan Howell <[email protected]> * Fix region parsing Signed-off-by: Ethan Howell <[email protected]> * Add length check Signed-off-by: Ethan Howell <[email protected]> * Update check Signed-off-by: Ethan Howell <[email protected]> --------- Signed-off-by: Ethan Howell <[email protected]> Signed-off-by: kunal-511 <[email protected]>
Thank you for contributing to Harbor!
Comprehensive Summary of your change
This PR extends the ecr regex check for c2s regions that don't follow the amazonaws domain convention.
Issue being fixed
Fixes #(issue)
Please indicate you've done the following: