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 ability to publish to channels #72
Conversation
|
Test PASSed. |
|
LGTM |
rogpeppe
reviewed
Mar 16, 2016
| +// Publish tells the charmstore to mark the given charm as published with the | ||
| +// given resource revisions to the given channels. If no channels are specified, | ||
| +// the stable channel will be assumed. | ||
| +func (s *CharmStore) Publish(id *charm.URL, channels []string, resources map[string]int) (*charm.URL, error) { |
rogpeppe
Mar 16, 2016
Owner
Why not just make this take a []params.Channel as an argument?
Then there's no need to copy the slice.
natefinch
Mar 16, 2016
Contributor
The point of the wrapper is to hide the ugly details of the HTTP API. The params package should be an implementation detail, IMO... that way you're not letting the API details leak outside the wrapper. Given that you're generally going to be using this from a CLI where we have to translate between a plain string and the API type anyway... seems like we might as well hide it away as far as possible.
rogpeppe
reviewed
Mar 16, 2016
| -// Publish tells the charmstore to mark the given charm as published with the given resource revisions. | ||
| -func (s *CharmStore) Publish(id *charm.URL, resources map[string]int) (*charm.URL, error) { | ||
| +// Publish tells the charmstore to mark the given charm as published with the | ||
| +// given resource revisions to the given channels. If no channels are specified, |
rogpeppe
Mar 16, 2016
Owner
I'm not entirely sure if we should do the "no channels==stable" thing at this level. It seems to me like that's more a policy decision for the outer level to take. This method could work OK as a very thin layer over the actual API endpoint, with no logic to speak of.
rogpeppe
reviewed
Mar 16, 2016
| @@ -177,12 +177,13 @@ func (ResourceSuite) TestPublish(c *gc.C) { | ||
| "data": 1, | ||
| "lib": 2, | ||
| } | ||
| - result, err := s.Publish(id, resources) | ||
| + result, err := s.Publish(id, []string{"development"}, resources) |
rogpeppe
Mar 16, 2016
Owner
You should probably test the "no channels == stable" thing. Or just delete that logic, which I think I prefer.
natefinch
Mar 16, 2016
Contributor
I agree with dropping the default to stable in this layer, will do.
rogpeppe
reviewed
Mar 16, 2016
| + } | ||
| + for i, ch := range channels { | ||
| + chans[i] = params.Channel(ch) | ||
| + } | ||
| val := ¶ms.PublishRequest{ | ||
| Published: true, |
rogpeppe
Mar 16, 2016
Owner
Delete this - it's ignored by the current API (and note TODO comment on the field in Publish).
You'll need to update the dependencies to use the latest charmstore version (which you should
do anyway so you're testing against a version that actually implements the functionality
we're trying to test).
natefinch
Mar 16, 2016
Contributor
I noticed last night that it wasn't being used elsewhere... I'll remove it.
rogpeppe
reviewed
Mar 16, 2016
| @@ -177,12 +177,13 @@ func (ResourceSuite) TestPublish(c *gc.C) { | ||
| "data": 1, | ||
| "lib": 2, | ||
| } | ||
| - result, err := s.Publish(id, resources) | ||
| + result, err := s.Publish(id, []string{"development"}, resources) | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| c.Assert(result, gc.DeepEquals, idrev) | ||
| f.CheckCall(c, 0, "PutWithResponse", "/"+id.Path()+"/publish", ¶ms.PublishRequest{ |
rogpeppe
Mar 16, 2016
Owner
Unfortunately this isn't actually checking what you think it's checking.
It's running the Publish call against the old (pre-new-channels-model)
charmstore, and it's "working" because the Publish field in the request
is set, which causes the previously uploaded charm id to be made available
in its non-development form. Well, you're not even checking whether the
charm has actually been published, but if you did, it would seem to work,
but if you tried retrieving the charm with the stable channel, it would
work even though the charm has only been published to development.
Let's make this test actually test whether the charm has been published
appropriately please, and that it has not been published to stable.
Again, you'll need to update the charmstore dep and fix the resulting
test failures.
natefinch
Mar 16, 2016
Contributor
Added a full stack test that publishes to development then tries to get from dev and stable (obv checking that it fails when retrieving from stable).
|
LGTM with a bunch of suggestions. You're in luck - I updated charmrepo to fix that charmstore dependency (PR #73) so if you rebase on that, you should be OK to have some working tests. |
natefinch
added some commits
Mar 15, 2016
|
Test PASSed. |
|
Test PASSed. |
|
|
|
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmrepo-merge |
added a commit
that referenced
this pull request
Mar 16, 2016
natefinch commentedMar 15, 2016
No description provided.