/charmrepo

create resource funcs #65

Merged
merged 8 commits into from Mar 11, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@natefinch @jujugui @ericsnowcurrently @kat-co
@natefinch
Contributor

natefinch commented Mar 4, 2016

No description provided.

@jujugui
Member

jujugui commented Mar 4, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/170/
Test PASSed.
csclient/csclient.go
@@ -156,6 +158,126 @@ func (c *Client) GetArchive(id *charm.URL) (r io.ReadCloser, eid *charm.URL, has
return resp.Body, eid, hash, resp.ContentLength, nil
}
+// ListResources retrieves the metadata about resources for the given charm.
+func (c *Client) ListResources(ids ...*charm.URL) (map[string][]params.Resource, error) {
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

Why the ...? We aren't supporting "give me all charms". And there isn't any precedent here, is there? (charmrepo.CharmStore.Latest doesn't count.)

@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

params.Resource threw me off for a sec, but it makes sense now.

csclient/csclient.go
+// GetResource retrieves the archive for the given charm or bundle, returning a
+// reader its data can be read from, the fully qualified id of the
+// corresponding entity, the SHA384 hash of the data and its size.
+func (c *Client) GetResource(id *charm.URL, name string) (result ResourceData, err error) {
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

revision must be passed in rather than returned (in ResourceData). If revision < 0 then we use the latest one.

csclient/csclient.go
+// ResourceData
+type ResourceData struct {
+ io.ReadCloser
+ Revision int
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

This is fine (for the case that revision < 0 is sent in the request).

csclient/params/params.go
+
+ // ResourceRevisionHeader specifies the header attribute that will hold the
+ // revision of the resource retrieved by a resource GET request.
+ ResourceRevisionHeader = "Resource-Revision"
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

Headers aren't meant to be used for query fields. Furthermore, the revision should be part of the URL path (in the download request), not a query field.

@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

Ah, it's for the response. Yuck. So either it must be in a header, or we use multi-part response bodies, or we make revision required.

@natefinch

natefinch Mar 4, 2016

Contributor

yeah, I'm just following what we do for returning the entity ID (see above where we get it from a header too).

csclient/params/params.go
+ Revision int
+ Fingerprint string // the hex encoded hash
+ Size int64
+}
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

You're missing Origin. We have not ruled out supporting more than just OriginStore in the store. Having the field here would make the information explicit and keep it in line with resource.Resource. It would also mean we wouldn't have to change the wire format later when Origin is more significant.

@natefinch

natefinch Mar 4, 2016

Contributor

Ahh, ok, yeah, I left it out, since OriginStore seemed implied. I can add it, that's fine.

csclient/params/params.go
+// Resource holds the metadata about a Resource defined for a charm.
+type Resource struct {
+ Name string
+ Type ResourceType
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

Why not just string? This is a wire type. Stricter bounds checking is facilitated through the helpers in charm/resource/type.go.

@natefinch

natefinch Mar 4, 2016

Contributor

Because it's basically free, and it makes the API much more descriptive. If we don't define the acceptable values in the API, then consumers will have to look at the implementation of our code to figure out what the valid values are.

@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

That seems like an argument for a helper then (e.g. the one we already have). :)

@natefinch

natefinch Mar 4, 2016

Contributor

Well... no, I mean, the type as a part of the API struct helps tell a consumer of the API what the API uses. Yes, for our specific code, the code does all the right stuff... but I'm talking about the structs as a way to document the actual HTTP API.

@natefinch

natefinch Mar 11, 2016

Contributor

note this is now a string since your stuff landed first. I still think it's wrong (note the magic strings in the api conversion functions).. but I'm too tired to really care :)

charmstore.go
@@ -232,6 +233,94 @@ func (s *CharmStore) Latest(curls ...*charm.URL) ([]CharmRevision, error) {
return responses, nil
}
+func (s *CharmStore) ListResources(ids ...*charm.URL) ([]ResourceResult, error) {
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

s/..._charm.URL/[]_charm.URL/

@natefinch

natefinch Mar 4, 2016

Contributor

This just follows how Latest works. It's not how I'd do it, but it's not my codebase.

@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

I think we have enough wiggle room to take a slice here. Latest() isn't the best example.

charmstore.go
+ return result, nil
+}
+
+func (c *CharmStore) UploadResource(id *charm.URL, name, filename string) (revision int, err error) {
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

You're missing the io.Reader arg.

@natefinch

natefinch Mar 4, 2016

Contributor

This is just the metadata.. there's no io.Reader, unless I'm not understanding something.

@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

This comment refers to UploadResource(), which is actually uploading the resource file.

charmstore.go
+ Fingerprint resource.Fingerprint
+}
+
+func (c *CharmStore) GetResource(id *charm.URL, name string) (result ResourceData, err error) {
@ericsnowcurrently

ericsnowcurrently Mar 4, 2016

Contributor

You're missing the resource revision field.

@jujugui
Member

jujugui commented Mar 4, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/171/
Test FAILed.
@jujugui
Member

jujugui commented Mar 4, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/172/
Test PASSed.
@jujugui
Member

jujugui commented Mar 7, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/178/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/181/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/182/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/183/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/184/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/185/
Test FAILed.
@jujugui
Member

jujugui commented Mar 8, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/186/
Test PASSed.

@natefinch natefinch changed the title from WIP: create resource funcs to create resource funcs Mar 8, 2016

@jujugui
Member

jujugui commented Mar 8, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/188/
Test PASSed.
@ericsnowcurrently
Contributor

ericsnowcurrently commented Mar 9, 2016

Make sure to also merge this into the new-channels-model branch.
charmstore.go
+
+ // User and Password hold the authentication credentials
+ // for the client. If User is empty, no credentials will be
+ // sent.
@kat-co

kat-co Mar 9, 2016

Contributor

I think you need to separate this comment to be above their respective member variables.

@natefinch

natefinch Mar 11, 2016

Contributor

I stole this exactly from the csclient code.. but I agree that it's wrong, so I'll fix it in both places :)

charmstore.go
+ }
+ list := make([]resource.Resource, len(resources))
+ for j, res := range resources {
+ resource, err := apiResource2Resource(res)
@kat-co

kat-co Mar 9, 2016

Contributor

Do we already have this function defined in core?

@natefinch

natefinch Mar 11, 2016

Contributor

Different API... that's the controller API. In theory, we could make them look mostly the same, but I think there's always going to be differences, so I'm not sure it's worth trying to shoehorn them into the same mold.

charmstore.go
+}
+
+// ResourceData holds the information about the bytes of a resource.
+type ResourceData struct {
@kat-co

kat-co Mar 9, 2016

Contributor

Should we be reusing the existing type defined in juju/charm?

@natefinch

natefinch Mar 11, 2016

Contributor

The one we have defined is in juju/juju ... so I don't think we can/should reference that from here. If it were in juju/charm, then maybe, though we need to return the revision, which is not included in that other type. Really, it's just a dumb data bag to avoid returning 4 values, and it has no functionality, so I don't think we need to worry about duplicating code here.

@kat-co
Contributor

kat-co commented Mar 9, 2016

Mostly LGTM, just had a few questions around the DRY principle.
@jujugui
Member

jujugui commented Mar 11, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/196/
Test PASSed.
@jujugui
Member

jujugui commented Mar 11, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmrepo/197/
Test PASSed.
@natefinch
Contributor

natefinch commented Mar 11, 2016

:shipit:
@jujugui
Member

jujugui commented Mar 11, 2016

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmrepo-merge

jujugui added a commit that referenced this pull request Mar 11, 2016

@jujugui
Merge pull request #65 from natefinch/resource-funcs 
create resource funcs
a770e0d

@jujugui jujugui merged commit a770e0d into juju:v2-unstable Mar 11, 2016
1 check passed

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment