Switch Radius Helm chart pull from ACR to GHCR #7455
Conversation
willdavsmith
force-pushed
the
willdavsmith/readd-ghcr
branch
from
b080257
to
ff6aec9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7455 +/- ##
==========================================
- Coverage 60.95% 60.95% -0.01%
==========================================
Files 518 518
Lines 26673 26690 +17
==========================================
+ Hits 16259 16269 +10
- Misses 8972 8981 +9
+ Partials 1442 1440 -2 ☔ View full report in Codecov by Sentry. |
pkg/cli/helm/helm.go
Outdated
| // https://github.com/helm/helm/issues/12584 | ||
| var errUnexpectedStatus containerdErrors.ErrUnexpectedStatus | ||
| if errors.As(err, &errUnexpectedStatus) { | ||
| unwrappedErr := UnwrapAll(err) |
There was a problem hiding this comment.
is there a better way to do this?
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
a note about the code coverage - the majority of changes in this PR are not tested by our unit tests and have not been so far |
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
willdavsmith
had a problem deploying
to
functional-tests
GitHub Actions
Failure
| } else { | ||
| // For OCI registries (like radius), we will use the | ||
| // repo URL + the releaseName as the chartRef. | ||
| // pull.Run("oci://ghcr.io/radius-project/helm-chart/radius") |
There was a problem hiding this comment.
Should we check if repoUrl is of this type oci://ghcr.io/radius-project?
There was a problem hiding this comment.
This code should work for all oci registries if we need to add them in the future. So I don't think we need to hard code it. the registry.IsOCI() call should determine if it's an oci registry or not.
pkg/cli/helm/helm.go
Outdated
| for { | ||
| unwrapped := errors.Unwrap(err) | ||
| if unwrapped == nil { | ||
| return err | ||
| } | ||
| err = unwrapped | ||
| } |
There was a problem hiding this comment.
This is to unwrap until you can't find any more errors, right?
There was a problem hiding this comment.
I'm not sure if there's a better way to do this though
There was a problem hiding this comment.
what's the problem you're trying to solve by doing this?
There was a problem hiding this comment.
It looks like you're doing an errors.As and then ignoring the result ....
There was a problem hiding this comment.
yep this is exactly what I was doing. no idea how I missed that functionality for errors.As()....
Signed-off-by: willdavsmith <willdavsmith@gmail.com>
Signed-off-by: willdavsmith <willdavsmith@gmail.com>
Signed-off-by: willdavsmith <willdavsmith@gmail.com>
ytimocin
force-pushed
the
willdavsmith/readd-ghcr
branch
from
6946703
to
1878b92
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
pkg/cli/helm/helm.go
Outdated
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
We need to also handle the case where the error is NOT an containerderrors.ErrUnexpectedStatus. Think about a case where the user is not connected to the internet.
Every time we special case one type of error, we need to also handle the case where it's some other type of error.
There was a problem hiding this comment.
Oh, nevermind. I see you did that above.
There was a problem hiding this comment.
I'd suggest putting this line in here as well for clarity: https://github.com/radius-project/radius/pull/7455/files#diff-35b8f26ef00e63853d6a8303dfdb8755355d7cb6d48ba9449d5a04af62a723cbR142
pkg/cli/helm/helm.go
Outdated
| var errUnexpectedStatus containerderrors.ErrUnexpectedStatus | ||
| if errors.As(err, &errUnexpectedStatus) { | ||
| if errUnexpectedStatus.StatusCode == http.StatusForbidden && strings.Contains(errUnexpectedStatus.RequestURL, "ghcr.io") { | ||
| return fmt.Errorf("recieved 403 unauthorized when downloading helm chart from the registry. you may want to perform a `docker logout ghcr.io` and re-try the command") |
There was a problem hiding this comment.
I'd suggest using clierrors.Message for this.
pkg/cli/helm/helm.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("error downloading helm chart from the registry for version: %s, release name: %s. Error: %w", version, releaseName, err) |
There was a problem hiding this comment.
I'd suggest using clierrors.Message for this. rad has special formatting for that type.
Basically it's helpful to know the difference between:
- You made a mistake, and it's not a bug in Radius. (
clierrors.Message) - Something weird happened, no idea 🤷 (any other error)
- awesome, thanks
willdavsmith
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
Example outputs:
Expired credentials:
Type of change
Fixes: #6801