chore: Improve external image provider for ImagePuller#2101
Conversation
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
requested review from
SDawley,
dkwon17,
ibuziuk and
mkuznyetsov
as code owners
|
/retest |
|
|
||
| func NewExternalImagesProvider() *ExternalImagesProvider { | ||
| p := &ExternalImagesProvider{ | ||
| imagesFilePath: externalImagesStoreFilePath, |
There was a problem hiding this comment.
nit, hardcoding /tmp doesn't look ideal. If you think it's okay maybe :
| imagesFilePath: externalImagesStoreFilePath, | |
| imagesFilePath: filepath.Join(os.TempDir(), externalImagesStoreFileName) |
| func fetchRawData(url string) ([]byte, error) { | ||
| client := &http.Client{ | ||
| Transport: &http.Transport{}, | ||
| Timeout: time.Second * 1, |
There was a problem hiding this comment.
Not related to this PR, I see it's coming from refactored changes from defaultimages.go; But isn't 1 second timeout a bit short? Maybe we can increase it?
|
|
||
| url := sample["url"] | ||
| if url != nil { | ||
| urls = append(urls, url.(string)) |
There was a problem hiding this comment.
Just a minor thought: we’re using a direct type assertion here, which could be a bit brittle.
Should we move to a guarded ok check to prevent any unexpected panics down the road?
| func getDashboardEditorsInternalAPIUrl(ctx *chetypes.DeployContext) string { | ||
| dashboardNamespace := ctx.CheCluster.Namespace | ||
| dashboardServiceName := defaults.GetCheFlavor() + "-dashboard" | ||
| return fmt.Sprintf("http://%s.%s.svc:8080/dashboard/api/editors", dashboardServiceName, dashboardNamespace) | ||
| } | ||
|
|
||
| func getDashboardSamplesInternalAPIUrl(ctx *chetypes.DeployContext) string { | ||
| dashboardNamespace := ctx.CheCluster.Namespace | ||
| dashboardServiceName := defaults.GetCheFlavor() + "-dashboard" | ||
| return fmt.Sprintf("http://%s.%s.svc:8080/dashboard/api/airgap-sample", dashboardServiceName, dashboardNamespace) | ||
| } |
There was a problem hiding this comment.
nit, minor duplication
| func getDashboardEditorsInternalAPIUrl(ctx *chetypes.DeployContext) string { | |
| dashboardNamespace := ctx.CheCluster.Namespace | |
| dashboardServiceName := defaults.GetCheFlavor() + "-dashboard" | |
| return fmt.Sprintf("http://%s.%s.svc:8080/dashboard/api/editors", dashboardServiceName, dashboardNamespace) | |
| } | |
| func getDashboardSamplesInternalAPIUrl(ctx *chetypes.DeployContext) string { | |
| dashboardNamespace := ctx.CheCluster.Namespace | |
| dashboardServiceName := defaults.GetCheFlavor() + "-dashboard" | |
| return fmt.Sprintf("http://%s.%s.svc:8080/dashboard/api/airgap-sample", dashboardServiceName, dashboardNamespace) | |
| } | |
| func getDashboardBaseInternalURL(ctx *chetypes.DeployContext) string { | |
| namespace := ctx.CheCluster.Namespace | |
| serviceName := defaults.GetCheFlavor() + "-dashboard" | |
| return fmt.Sprintf("http://%s.%s.svc:8080/dashboard/api", serviceName, namespace) | |
| } | |
| func getDashboardEditorsInternalAPIUrl(ctx *chetypes.DeployContext) string { | |
| return fmt.Sprintf("%s/editors", getDashboardBaseInternalURL(ctx)) | |
| } | |
| func getDashboardSamplesInternalAPIUrl(ctx *chetypes.DeployContext) string { | |
| return fmt.Sprintf("%s/airgap-sample", getDashboardBaseInternalURL(ctx)) | |
| } |
| expectedFileContent := "" | ||
| for i, img := range tc.expectedImages { | ||
| if i > 0 { | ||
| expectedFileContent += "\n" | ||
| } | ||
| expectedFileContent += img | ||
| } |
There was a problem hiding this comment.
The test reconstructs file content using logic similar to the implementation (strings.Join). Do you think we can use this?
| expectedFileContent := "" | |
| for i, img := range tc.expectedImages { | |
| if i > 0 { | |
| expectedFileContent += "\n" | |
| } | |
| expectedFileContent += img | |
| } | |
| expectedFileContent := strings.Join(tc.expectedImages, "\n") |
|
@rohanKanojia |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
What does this PR do?
Refactors the ImagePuller's image fetching code: renames DashboardApiDefaultImagesProvider to ExternalImagesProvider, improves error handling (failures now block reconciliation by default instead of being silently
ignored), adds proper HTTP response body cleanup and status code checking.
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
eclipse-che/che#23686
How to test this PR?
OpenShift
or
on Minikube
Common Test Scenarios
PR Checklist
As the author of this Pull Request I made sure that:
Reviewers
Reviewers, please comment how you tested the PR when approving it.