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
Implement the bundles-containing API endpoint. #78
Conversation
Test PASSed. |
@@ -228,6 +228,9 @@ func bundleCharms(data *charm.BundleData) ([]*charm.Reference, error) { | |||
return nil, errgo.Mask(err) | |||
} | |||
urlMap[url.String()] = url | |||
// Also add the corresponding base URL. | |||
base := baseURL(url) | |||
urlMap[base.String()] = base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just:
urlMap[base.String() = baseURL(url)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow this, we need to define base to be able to do base.String()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Ignore me :-)
LGTM, with some small remarks (validation and "flat" names). |
Very nice stuff, thanks. LGTM modulo the above comments and suggestions. |
Test PASSed. |
Thanks for the reviews! |
… if flags are invalid.
…taining API endpoint.
Test PASSed. |
Test PASSed. |
👍 |
Test PASSed. |
case "1": | ||
return true, nil | ||
} | ||
return false, errgo.Newf(`value must be either "0", "1" or empty, but %q passed`, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errgo.Newf(`unexpected bool value %q (must be "0" or "1")`, value)
?
(probably not worth mentioning that it can be empty - that's just to allow
it to be omitted)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
A few more suggestions, but still LGTM. |
Test PASSed. |
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmstore-merge |
Implement the bundles-containing API endpoint.
No description provided.