Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error message on invalid scheme-prefixed ref #185

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Conversation

relu
Copy link
Member

@relu relu commented Oct 25, 2021

Adds a descriptive error when an invalid image ref containing a scheme
is given to the ImageRepository.

Fixes #146

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit, othewise lgtm

controllers/scan_test.go Outdated Show resolved Hide resolved
@relu relu force-pushed the err-invalid-image-ref branch from 7acdbb2 to 8f919d4 Compare October 27, 2021 10:09
@relu relu force-pushed the err-invalid-image-ref branch from 8f919d4 to b7c65b7 Compare October 27, 2021 10:10
Eventually(func() bool {
_ = r.Get(ctx, objectName, &repo)
ready = apimeta.FindStatusCondition(*repo.GetStatusConditions(), meta.ReadyCondition)
fmt.Printf("%v", ready)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left-over debugging println?

@@ -143,6 +143,9 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ

ref, err := name.ParseReference(imageRepo.Spec.Image)
if err != nil {
if u, _ := url.Parse(imageRepo.Spec.Image); u != nil && u.Scheme != "" {
err = fmt.Errorf("image repository attribute should not be scheme-prefixed ('%s://')", u.Scheme)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err = fmt.Errorf("image repository attribute should not be scheme-prefixed ('%s://')", u.Scheme)
err = fmt.Errorf(".spec.image value should not be scheme-prefixed ('%s://')", u.Scheme)

(i.e., refer to the field by path)

@relu relu force-pushed the err-invalid-image-ref branch from b7c65b7 to e46d73d Compare October 27, 2021 15:17
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only a suggestion on the language, which I leave to your discretion -- thanks Aurel 🍍

controllers/imagerepository_controller.go Outdated Show resolved Hide resolved
@relu relu force-pushed the err-invalid-image-ref branch from e46d73d to 2f9119e Compare October 27, 2021 15:29
Adds a descriptive error when an invalid image ref containing a scheme
is given to the ImageRepository.

Fixes #146

Signed-off-by: Aurel Canciu <[email protected]>
@relu relu force-pushed the err-invalid-image-ref branch from 2f9119e to b745ca8 Compare November 30, 2021 13:31
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks.

@relu relu merged commit 074cbc9 into main Dec 1, 2021
@relu relu deleted the err-invalid-image-ref branch December 1, 2021 20:26
@squaremo
Copy link
Member

Oh -- except parsing image repositories (no scheme, but port numbers with a colon) as though they are URLs (scheme with a colon expected) is going to give spurious errors: #220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth for "https:" not found
4 participants