Add support for Google Artifact Registry #1377
Conversation
cw75
requested review from
likawind,
saurav-c,
hsubbaraj-spiral and
jpurusho65
and removed request for
hsubbaraj-spiral
| @@ -80,7 +80,7 @@ class APIClient: | |||
| GET_DYNAMIC_ENGINE_STATUS_ROUTE = "/api/resource/dynamic-engine/status" | |||
| EDIT_DYNAMIC_ENGINE_ROUTE_TEMPLATE = "/api/resource/dynamic-engine/%s/edit" | |||
|
|
|||
| GET_IMAGE_URL_ROUTE = "/api/resourece/container-registry/url" | |||
There was a problem hiding this comment.
do you see any other this typo in the code base?
| return getImageURLResponse{ | ||
| Url: args.imageName, | ||
| }, http.StatusOK, nil | ||
| } else if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
I'd generally prefer not use else if the previous if block returns. We can just use a series of if blocks.
| image := strings.Split(fullImageUrl, "/")[3] | ||
|
|
||
| // Create a new HTTP request against the Google Artifact Registry API | ||
| req, err := http.NewRequest("GET", fmt.Sprintf("https://%s/v2/%s/%s/%s/manifests/%s", host, projectID, repo, image, tag), nil) |
There was a problem hiding this comment.
Does GAR has any go library that we can use? Nowadays I'd imagine most such endpoints has go clients and shouldn't require us to implement raw http requests
There was a problem hiding this comment.
Yeah I tried multiple attempts but Google's container registry client API seems broken, and multiple sources suggest that we just use the HTTP APIs here..
There was a problem hiding this comment.
lol that's really badly maintained
| return emptyResponse, resp.StatusCode, errors.Newf("Received unexpected status: %d", resp.StatusCode) | ||
| } | ||
| } else { | ||
| return emptyResponse, http.StatusBadRequest, errors.Newf("Container registry service %s is not supported.", args.service) |
There was a problem hiding this comment.
I may misread, but it seems we will fall at this line if service is GAR?
There was a problem hiding this comment.
If the service is GAR, we will hit line 117 } else if args.service == shared.GAR { right? This line is for services other than ECR and GAR.
There was a problem hiding this comment.
To make it clearer, I removed the else statements since we're returning in each case.
cw75
added
the
skip_integration_test
Describe your changes and why you are making these changes
This PR enables support for GAR. The APIs on the UI and SDK are exactly the same as ECR, but expecting GCP credentials (service account key in our case).
Manually verified that both the success case and failure cases (invalid credentials, non-existent images) work.
Related issue number (if any)
Loom demo (if any)
Checklist before requesting a review
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)