Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add an API data type for resources. #68
Conversation
ericsnowcurrently
added some commits
Mar 7, 2016
|
Test PASSed. |
natefinch
reviewed
Mar 8, 2016
| + // Size is the size of the resource, in bytes. | ||
| + Size int64 | ||
| +} | ||
| + | ||
| const ( |
natefinch
Mar 8, 2016
Contributor
They explicitly don't lowercase the json in this package... we should follow suit. (also, IMO, it's better not to anyway).
natefinch
reviewed
Mar 8, 2016
| +) | ||
| + | ||
| +// Resource2API converts a charm resource into an API Resource struct. | ||
| +func Resource2API(res resource.Resource) Resource { |
natefinch
Mar 8, 2016
Contributor
I'm not convinced this is the right package for the conversion helper. I think it's more appropriate in csclient or even the root charmrepo package. I think the whole point is for the params package to only know or care about API params.
natefinch
Mar 8, 2016
Contributor
I take it back... given that we need a helper for both sides of the API... I guess it has to be here. Kinda wish it could live somewhere else and just import params... but I don't think it's worth creating a new package at this time.
mhilton
Mar 9, 2016
Member
It'd be nice is a resource.Resource was just marshalable without the need for conversion. I assume there are solid reasons why it isn't though.
|
Test PASSed. |
|
LGTM |
frankban
reviewed
Mar 9, 2016
| + | ||
| + rtype, err := resource.ParseType(apiInfo.Type) | ||
| + if err != nil { | ||
| + return res, err |
frankban
reviewed
Mar 9, 2016
| + err = expected.Validate() | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + c.Check(res, jc.DeepEquals, expected) |
frankban
Mar 9, 2016
Member
What about testing the error cases, invalid type and origin?
Also a test for the case when the fingerprint is empty?
|
|
mhilton
reviewed
Mar 9, 2016
| + if len(fpSum) == 0 { | ||
| + return resource.Fingerprint{}, nil | ||
| + } | ||
| + return resource.NewFingerprint(fpSum) |
mhilton
Mar 9, 2016
Member
I generally associate New with creating a pointer to a thing. This one produces a thing, I expect there is a more common go idiom for this, but I can't think of it right now.
|
|
|
Test PASSed. |
ericsnowcurrently
added some commits
Mar 9, 2016
|
Test PASSed. |
|
|
|
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmrepo-merge |
added a commit
that referenced
this pull request
Mar 9, 2016
jujugui
merged commit e6b798e
into
juju:v2-unstable
Mar 9, 2016
1 check passed
ericsnowcurrently
deleted the
ericsnowcurrently:resources-resource-api-type
branch
Mar 9, 2016
ericsnowcurrently
restored the
ericsnowcurrently:resources-resource-api-type
branch
Mar 9, 2016
ericsnowcurrently
referenced this pull request
Mar 9, 2016
Merged
Add an API data type for resources. #69
added a commit
that referenced
this pull request
Mar 9, 2016
ericsnowcurrently commentedMar 7, 2016
This provides a stable over-the-wire format that maps onto the charm/resource.Resource type.