/juju

Extract resources apiserver parts into the apiserver package #7123

Merged
merged 23 commits into from Mar 21, 2017

Conversation

Reviewers

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjs @jujubot @wallyworld
@mjs
Contributor

mjs commented Mar 20, 2017

Description of change

This PR is part of the cleanup of the apiserver facade registration. All the resources related apiserver code has been moved to the apiserver package, include HTTP endpoints. This means that resources related APIs are now defined in the same way as all other APIs.

Some highlights:

  • The Resources and ResourceHookContext facades now exist under apiserver.
  • Some mime handling backport code for Go < 1.6 has been removed. We require at least Go 1.7 now.
  • Client and unit HTTP endpoints for resource upload and download have been moved to the apiserver package.
  • Many layers of completely unnecessary and confusing abstraction have been removed.
  • Removed the now-unused apiserver plumbing to support for the component architecture. Both the payloads and resources features have been extracted to apiserver now.
  • Drive-by test fixes for Go 1.8.

This PR results in a net removal of over 1700 lines of code, for no functional change!

QA steps

$ juju deploy cs:~cmars/mattermost
# Confirmed that deploy works and resource downloads on to unit.

$ juju list-resources mattermost                          
[Service]
Resource  Supplied by  Revision
bdist     charmstore   3

# Now test with the dummy-resource charm from the QA team

$ cd ~/canonical/repository/charms/dummy-resource
$ juju deploy . --resource foo=foo.txt --resource bar=bar.txt
# Charm emits resource info as status. Confirmed that resource was deployed.

$ juju status | grep Path:
dummy-resource/0*  maintenance  idle   1        10.0.8.199             Path: /var/lib/juju/agents/unit-dummy-resource-0/resources/bar/bar.txt Time: 0 sec Size: 17 bytes

# Update the resource
$ juju attach dummy-resource bar=foo.txt

$ Confirm that the resource is updated
$ juju status | grep Path:
dummy-resource/0*  maintenance  idle   1        10.0.8.199             Path: /var/lib/juju/agents/unit-dummy-resource-0/resources/bar/bar.txt Time: 0 sec Size: 27 bytes
# (note the change in size)

$ juju list-resources dummy-resource
[Service]
Resource  Supplied by  Revision
foo       admin        2017-20-03T00:03
bar       admin        2017-20-03T00:13

Documentation changes

No user visible effects.

Bug reference

N.A.

mjs added some commits Mar 8, 2017

@mjs
Extract Resources facade (component to apiserver) 
Have the facade defined out of the apiserver package causes many
problems in terms of ongoing maintenance.
e5b64ba
@mjs
Make resources tests pass again 
This is a partial rationalisation of the resources component code now
that the apiserver parts have moved to under the apiserver package.
1c63b65
@mjs
Remove pre Go 1.6 mime code from resources 
resources/api included some backported MIME functionality for versions
of Go from before 1.6. This is no longer needed as Juju is only
supported on 1.6 and later now.
503cf3d
@mjs
resource/api/server: Remove dead test code
33f0135
@mjs
resource/api/client: Clean up helpers 
Don't export helpers which are not referred to outside the package. Move
helpers from thier own file into the file where they're used.
d0de669
@mjs
Move uniter resources params to apiserver/params 
This is part of extracting the resources apiserver parts.
9be079c
@mjs
Move resource unit hook context to apiserver 
This is part of extracting the resources API facdes to the apiserver
package (like all other facades).
2a03921
@mjs
apiserver: Better name for resources endpoints 
The HTTP endpoints for resources are migration specific so name them and
the files as such.
8843d84
@mjs
Extract resource up/download functionality 
Move all this out from resource/api/server to apiserver. Much
unnecessary abstraction and stupid test technique has also been removed.
bf6f948
@mjs
Move uniter resource endpoint to apiserver 
This is the last component architecture apiserver part to be
moved. Much unnecessary abstraction and dead code has been removed. The
code is now much more straightforward.
61010bf
@mjs
resource: Removed unnecessary Closer support 
This is no longer used.
de8be34
@mjs
apiserver/resources: Hide internal type 
Backend does not need to be exposed so don't.
ceb7331
@mjs
apiserver: Removed component arch plumbing 
The apiserver infrastrcture to support the component architecture bits
are now unused and have been removed.
f8cc1b2
@mjs
Remove now-unnecessary calls to RegisterForServer 
The resources and payload facades are now always registered and don't
require the component mechanism to do this.
41fc8e0
@mjs
apiserver: Register ResourcesUnitContext facade
c443436
@mjs
api: Adjust tests for Go 1.8 
Some tests were testing for error messages which have changed in Go
1.8. The regexes have been made less specific.
c4792b8
@wallyworld

wallyworld approved these changes Mar 20, 2017
View changes

Looks pretty nice. So good to get this cleaned up.
Not sure if we want to test with a 2.0 client just to be sure.

apiserver/apiserver.go
@@ -475,7 +509,7 @@ func (srv *Server) endpoints() []apihttp.Endpoint {
},
)
add("/migrate/resources",
- &resourceUploadHandler{
+ &resourceMigUploadHandler{
@wallyworld

wallyworld Mar 20, 2017

Owner

I'd love to see "Mig" replaced with the full name rather than the abbreviation.
I thought of a jet fighter and took me a second to realise what it really was

@mjs

mjs Mar 20, 2017

Contributor

Fixed :)

apiserver/params/resources_unit.go
- "github.com/juju/juju/apiserver/params"
- "github.com/juju/juju/resource/api"
-)
+package params
// ListResourcesArgs holds the arguments for an API request to list
@wallyworld

wallyworld Mar 20, 2017

Owner

make comment match, here and below

@mjs

mjs Mar 20, 2017

Contributor

Fixed

apiserver/resources/facade.go
newCharmstoreClient func() (CharmStore, error)
}
-// NewFacade returns a new resoures facade for the given Juju state.
-func NewFacade(store DataStore, newClient func() (CharmStore, error)) (*Facade, error) {
+// NewPublicFacade creates apublic API facade for resources. It is
@wallyworld

wallyworld Mar 20, 2017

Owner

typo

@mjs

mjs Mar 20, 2017

Contributor

Fixed

apiserver/resources/facade.go
+}
+
+// NewFacade returns a new resoures API facade.
+func NewFacade(store backend, newClient func() (CharmStore, error)) (*Facade, error) {
@wallyworld

wallyworld Mar 20, 2017

Owner

Does this really need to be exported? I guess it is used in tests.

@mjs

mjs Mar 20, 2017

Contributor

Yep, for tests.

apiserver/resources/server_addpending_test.go
@@ -293,24 +292,24 @@ func (s *AddPendingResourcesSuite) TestWithURLUpload(c *gc.C) {
//func (s *AddPendingResourcesSuite) TestUnknownResource(c *gc.C) {
@wallyworld

wallyworld Mar 20, 2017

Owner

Not sure why this was originally commented out.
We should either skip with a bug and a todo, or delete.

@mjs

mjs Mar 20, 2017

Contributor

Not sure what the background here is but it looks like one of the other (enabled) tests already covers this.

apiserver/resources_mig.go
@@ -0,0 +1,164 @@
+// Copyright 2016 Canonical Ltd.
@wallyworld

wallyworld Mar 20, 2017

Owner

2017 if this is new

@mjs

mjs Mar 20, 2017

Contributor

Fixed

resource/resourceadapters/opener.go
+//
+// TODO(mjs): This is the entry point for a whole lot of untested shim
+// code in this package. At some point this should be sorted out.
+func NewResourceOpener(st *corestate.State, unitName string) (opener resource.Opener, err error) {
@wallyworld

wallyworld Mar 20, 2017

Owner

Can you add a comment that the caller is responsible for closing state just be make it explicit.
I traced to code to be sure we weren't leaking connections, so would be nice to inform the reader the expectation.

@mjs

mjs Mar 20, 2017

Contributor

Done

mjs added some commits Mar 20, 2017

@mjs
apiserver/params: Fix names in docstrings
22400fc
@mjs
apiserver: RenamelresourceMigUploadHandler 
resourcesMigrationUploadHandler is a longer but less fighter-jettish
name.
583fcb7
@mjs
apiserver/resources: Deleted commented out test 
Appears to already be covered by other tests.
1254b8a
@mjs
apiserver/resources: Expose Backend 
This is more in line with other facades. Ultimately we would like all
facade to not deal with State directly and just expose the backend
interfaces they need.
65609e0
@mjs
resource/resourceadapters: Clarify State ownership
cf6d468
@mjs
Contributor

mjs commented Mar 20, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 20, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 20, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10495
@mjs
Contributor

mjs commented Mar 20, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 20, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 20, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10496
@mjs
apiserver: Avoid post Go 1.6 httptest feature 
httptest.ResponseRecorder only got a Result method after Go 1.6.
234131a
@mjs
Contributor

mjs commented Mar 21, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 21, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 21, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10504
@mjs
Contributor

mjs commented Mar 21, 2017

$$mongodb-died$$
@jujubot
Contributor

jujubot commented Mar 21, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit f36b798 into juju:develop Mar 21, 2017

@mjs mjs deleted the mjs:extract-resources-apiserver branch Mar 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment