Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

Godeps: bump docker2aci #2197

Merged
merged 2 commits into from Feb 19, 2016
Merged

Godeps: bump docker2aci #2197

merged 2 commits into from Feb 19, 2016

Conversation

iaguis
Copy link
Member

@iaguis iaguis commented Feb 19, 2016

Fixes #2008
Closes #2134

dockerURL := d2acommon.ParseDockerURL(path.Join(u.Host, u.Path))
dockerURL, err := d2acommon.ParseDockerURL(path.Join(u.Host, u.Path))
if err != nil {
return "", fmt.Errorf("invalid docker URL %q. Syntax docker://[REGISTRY_HOST[:REGISTRY_PORT]/]IMAGE_NAME[:TAG]", u)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about wrapping the error gotten from docker2aci?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but I don't like the result: without --debug, we only print the first and last errors, that is: run: empty Docker image reference and I'd like to show the right syntax to the user.

Maybe we can modify the error in docker2aci, although it doesn't make sense to include the docker:// part because the function parses the part after the schema.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, fair enough.

Also, I'm not sure if I like the current error message being two sentences. How about:

fmt.Errorf(`invalid docker URL %q; syntax is "docker://blahblahblah"`, u)

Either semicolon or a comma and add a missing "is".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go with

fmt.Errorf(`invalid docker URL %q; expected syntax is "docker://[REGISTRY_HOST[:REGISTRY_PORT]/]IMAGE_NAME[:TAG]"`, u)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure.

@iaguis
Copy link
Member Author

iaguis commented Feb 19, 2016

  • Changelog

@krnowak
Copy link
Collaborator

krnowak commented Feb 19, 2016

I don't think this is worthy for a changelog entry. It's just a bugfix.

@krnowak
Copy link
Collaborator

krnowak commented Feb 19, 2016

Ok, so it needs a changelog entry for #2008. I thought this PR fixes only #2134, which is not serious enough to get an entry in changelog.

@iaguis
Copy link
Member Author

iaguis commented Feb 19, 2016

Ok, so it needs a changelog entry for #2008. I thought this PR fixes only #2134, which is not serious enough to get an entry in changelog.

👍

dockerURL := d2acommon.ParseDockerURL(path.Join(u.Host, u.Path))
dockerURL, err := d2acommon.ParseDockerURL(path.Join(u.Host, u.Path))
if err != nil {
return "", fmt.Errorf(`invalid docker URL %q; expected syntax "docker://[REGISTRY_HOST[:REGISTRY_PORT]/]IMAGE_NAME[:TAG]"`, u)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You swallowed the "is" word.

Copy link
Member Author

Choose a reason for hiding this comment

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

Achh

@krnowak
Copy link
Collaborator

krnowak commented Feb 19, 2016

LFAD.

krnowak added a commit that referenced this pull request Feb 19, 2016
@krnowak krnowak merged commit 6db58f6 into rkt:master Feb 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants