Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
charms HTTPS API: allow for providing the default charm icon. #6352
Conversation
frankban
referenced this pull request
in juju/juju-gui
Sep 29, 2016
Merged
Use new Juju 2 endpoint for getting the local charm icon. #2051
| @@ -78,7 +78,7 @@ func (h *charmsHandler) serveGet(w http.ResponseWriter, r *http.Request) error { | ||
| // Retrieve or list charm files. | ||
| // Requires "url" (charm URL) and an optional "file" (the path to the | ||
| // charm file) to be included in the query. |
| @@ -78,7 +78,7 @@ func (h *charmsHandler) serveGet(w http.ResponseWriter, r *http.Request) error { | ||
| // Retrieve or list charm files. | ||
| // Requires "url" (charm URL) and an optional "file" (the path to the | ||
| // charm file) to be included in the query. | ||
| - charmArchivePath, fileArg, err := h.processGet(r, st) | ||
| + charmArchivePath, fileArg, serveDefaultIcon, err := h.processGet(r, st) |
rogpeppe
Sep 30, 2016
Owner
Perhaps "serveIcon" would be a more appropriate name, as that's the name of the
query value and what we're really doing is asking for the icon, which a fallback to
the default icon only when needed.
| @@ -191,7 +193,15 @@ func (h *charmsHandler) archiveEntrySender(filePath string) bundleContentSenderF | ||
| io.Copy(w, contents) | ||
| return nil | ||
| } | ||
| - return errors.NotFoundf("charm") | ||
| + // Serve the default icon (if the charm icon was requested), or return | ||
| + // a 404 not found response. |
rogpeppe
Sep 30, 2016
Owner
I think this comment would read better inside the if statement:
if serveDefaultIcon {
// An icon was requested but none was found in the archive so
// return the default icon instead.
....
}
// No file found
| -func (h *charmsHandler) processGet(r *http.Request, st *state.State) (string, string, error) { | ||
| +// It returns the bundle path, the requested file path (if any), whether the | ||
| +// default charm icon has been requested and an error. | ||
| +func (h *charmsHandler) processGet(r *http.Request, st *state.State) (string, string, bool, error) { |
| query := r.URL.Query() | ||
| // Retrieve and validate query parameters. | ||
| curlString := query.Get("url") | ||
| if curlString == "" { | ||
| - return "", "", fmt.Errorf("expected url=CharmURL query argument") | ||
| + return "", "", false, fmt.Errorf("expected url=CharmURL query argument") |
rogpeppe
Sep 30, 2016
•
Edited 1 time
-
rogpeppe
Sep 30, 2016
Owner
-
Sep 30, 2016
I saw a nice idiom recently that can reduce the boilerplate in this kind
of case. Define a local variable:
errRet := func(err error) (string, string, bool, error) {
return "", "", false, err
}
then all your returns can just be:
return errRet(fmt.Errorf(....))
and if you want to change the return parameters, you just need to change it one
place. Useful for large value-type names too.
| + expectBody: "1", | ||
| + }} | ||
| + | ||
| + // Run the tests. |
| +package apiserver | ||
| + | ||
| +// defaultIcon holds the default charm icon SVG content. | ||
| +// Keep this in sync with the default icon returned by the charm store. |
frankban
Sep 30, 2016
Member
Perhaps, that would be interesting to implement. Although the default icon never changed, so it could be overkill.
| @@ -0,0 +1,335 @@ | ||
| +<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
rogpeppe
Sep 30, 2016
Owner
Can't we use an in-memory test charm rather than adding to the global charm repo charms?
Also, does the icon have to be the actual icon SVG - just a token amount of text would probably do.
frankban
Sep 30, 2016
Member
Given we have some charms there already I've found adding an icon simpler. We could convert all these tests to use in memory charms, I'd prefer not to do that in this charm. Also, I just aded the real mysql icon.
frankban
added some commits
Sep 30, 2016
|
Thank you for the reviews! |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
frankban commentedSep 29, 2016
This is done so that the GUI can safely user the Juju URL to the icon even in the case of local charms without an icon.
The API change here is backward compatible.