/juju

Resources export, serialisation & deserialisation #6492

Merged
merged 15 commits into from Oct 27, 2016

Conversation

Reviewers

@howbazaar howbazaar

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

mjs commented Oct 25, 2016 •
Edited 1 time

This pull request covers the export, serialisation and deserialisation of application resources.

Still to come:

  • import of resources back into state
  • handling of per unit resource revisions
  • export and import of resource binaries

QA

Visual inspection of juju dump-model output for a model with a charm which uses resources. More involved QA will be done once the above items have been handled.

Also tested a standard migration to ensure there's no regressions.
@mjs
Contributor

mjs commented Oct 25, 2016

!!try!!
@mjs
Contributor

mjs commented Oct 25, 2016

!!build!!
core/description/application.go
@@ -45,6 +45,8 @@ type application struct {
// unit count will be assumed by the number of units associated.
Units_ units `yaml:"units"`
+ Resources_ resources `yaml:"resources,omitempty"`
@howbazaar

howbazaar Oct 25, 2016

Owner

omitempty doesn't work on structs

@mjs

mjs Oct 26, 2016

Contributor

Fixed

core/description/application_test.go
@@ -71,6 +77,7 @@ func minimalApplication(args ...ApplicationArgs) *application {
u.SetAgentStatus(minimalStatusArgs())
u.SetWorkloadStatus(minimalStatusArgs())
u.SetTools(minimalAgentToolsArgs())
+ s.setResources([]*resource{minimalResource()})
@howbazaar

howbazaar Oct 25, 2016

Owner

Why not use AddResource with some args?

@mjs

mjs Oct 25, 2016

Contributor

b/c this way I can reuse minimalResource :)

core/description/application_test.go
+
+func (s *ApplicationSerializationSuite) TestResourcesAreValidated(c *gc.C) {
+ application := minimalApplication()
+ application.setResources([]*resource{
@howbazaar

howbazaar Oct 25, 2016

Owner

application.AddResource(ResourceArgs{Name: "foo"})

@mjs

mjs Oct 26, 2016

Contributor

Fixed

core/description/resource.go
+ Name() string
+ Revision() int
+ CharmStoreRevision() int
+ AddRevision(ResourceRevisionArgs)
@howbazaar

howbazaar Oct 25, 2016

Owner

In other places, the AddXXX method also returns the XXX

@mjs

mjs Oct 26, 2016

Contributor

Fixed

core/description/resource.go
+ Revision() int
+ CharmStoreRevision() int
+ AddRevision(ResourceRevisionArgs)
+ Revisions() map[int]ResourceRevision
@howbazaar

howbazaar Oct 25, 2016

Owner

Does it need to be a map? If the ResourceRevision contains the Revision value itself, perhaps we just have a sorted sequence of ResourceRevision?

@mjs

mjs Oct 25, 2016

Contributor

They're stored and serialised as a sequence but it's more convenient to return them as a map. The Validate method ensures that each revision only appears once.

core/description/resource.go
+ Path() string
+ Description() string
+ Origin() string
+ FingerprintHex() string
@howbazaar

howbazaar Oct 25, 2016

Owner

Do we need the 'Hex' suffix?

@mjs

mjs Oct 25, 2016

Contributor

I did this because on resource.Resource the fingerprint is stored as a raw []byte but here we convert it to a hex representation of that. The name is a reminder that a conversion has been performed.

core/description/resource.go
+ Origin() string
+ FingerprintHex() string
+ Size() int64
+ AddTimestamp() time.Time
@howbazaar

howbazaar Oct 25, 2016

Owner

Add is a verb, how about TimeAdded ? or just Timestamp?

@mjs

mjs Oct 27, 2016

Contributor

Done. I went with Timestamp

core/description/resource.go
+ Name_: args.Name,
+ Revision_: args.Revision,
+ CharmStoreRevision_: args.CharmStoreRevision,
+ Revisions_: make([]*resourceRevision, 0),
@howbazaar

howbazaar Oct 25, 2016

Owner

Why even make a zero length?

@mjs

mjs Oct 27, 2016

Contributor

Duh. Fixed.

core/description/resource.go
+ "username": schema.String(),
+ }
+ defaults := schema.Defaults{
+ "add-timestamp": time.Time{},
@howbazaar

howbazaar Oct 25, 2016

Owner

It would be easier to have schema.Omit for the timestamp, and then only set it below if it exists, rather than go through zero.

@mjs

mjs Oct 27, 2016

Contributor

Yep, that's slightly better. I'll make a helper and change all the other places in the package that have time pointers going via the zero value.

core/description/resource_test.go
+ }
+}
+
+func minimalResourceMap() map[interface{}]interface{} {
@howbazaar

howbazaar Oct 25, 2016

Owner

Hmm... this isn't a minimal one though is it?

@mjs

mjs Oct 27, 2016

Contributor

Close enough :) One revision and one charm store revision is fairly small and exercises most of the functionality.

featuretests/package_test.go
coretesting "github.com/juju/juju/testing"
)
var runFeatureTests = flag.Bool("featuretests", true, "Run long-running feature tests.")
func init() {
+ // Required for anything requiring components (e.g. resources).
+ if err := all.RegisterForServer(); err != nil {
@howbazaar

howbazaar Oct 25, 2016

Owner

This is really icky.

@mjs

mjs Oct 25, 2016

Contributor

The joys of the "component architecture".

resource/api/client/base_test.go
@@ -81,6 +81,7 @@ func newResource(c *gc.C, name, username, data string) (resource.Resource, api.R
apiRes := api.Resource{
CharmResource: api.CharmResource{
Name: name,
+ Description: name + " description",
@howbazaar

howbazaar Oct 25, 2016

Owner

Why were these needed?

@mjs

mjs Oct 25, 2016

Contributor

I wanted the resources in tests to have a description so that I could see the description being handled. Predictably basing it off the name was an easy way to achieve that.

state/migration_export.go
for _, application := range applications {
applicationUnits := e.units[application.Name()]
leader := leaders[application.Name()]
+ resources, err := resourcesSt.ListResources(application.Name())
@howbazaar

howbazaar Oct 25, 2016

Owner

Bah humbug. The only part of export where we do more than one query per collection.

@mjs

mjs Oct 25, 2016

Contributor

I might be able to avoid the extra lookups but it'll mean extending or going around the many levels of resources state API.

state/upgrade_test.go
@@ -5,8 +5,7 @@ package state_test
import (
"fmt"
- "time" // Only used for time types.
-
+ // Only used for time types.
@howbazaar

howbazaar Oct 25, 2016

Owner

Don't need this comment now.

@mjs

mjs Oct 25, 2016

Contributor

Interesting. goimports preserves comments when it removes an import.

@mjs

mjs Oct 27, 2016

Contributor

Fixed.

mjs added some commits Oct 18, 2016

@mjs
resource/resourcetesting: Desc for test resources
f230596
@mjs
state: Fixed docstring typo
9dbf267
@mjs
state: Add test helper for DB timestamps 
MongoDB only stores timestamps with millisecond precision so it's useful
to have a helper function for truncating them for comparison in tests.
5171186
@mjs
Migration export of resources 
Application and charmstore revisions done.
Unit revisions still to go.
0ecebdc
@mjs
core/description: Resource (de)serialisation 
Implemented unit tests for resource descriptions and deserialisation
support.
3c66100
@mjs
core/description: Serialise revisions as list 
This avoids doubling up of revision number and just makes everything a
whole lot easier.
264f5f8
@mjs
core/description: Wire up resource import in application
3a11e64
@mjs
core/description: Validate resources when validating applications
852d3cc
@mjs
migration: Register components in tests 
This is required to allow import testing of resources.
42136c4
@mjs
state: Removed reminders about unit revisions 
This will be handled in the next PR.
243925b
@mjs
Clarify that resource fingerprint is hex 
... in model description.
0e4fddb
@mjs
resource/api: Fix test expectations 
Resource test helpers now create resources with a description when they
didn't before.
439a519
@mjs
core/description: Misc review feedback 
- removed unnecessary omitempty
- AddRevision returns the new revision
- Rename AddTimestamp field to just Timestamp
2aabbd9
core/description/util.go
+
+import "time"
+
+func fieldToTimePtr(fields map[string]interface{}, name string) *time.Time {
@howbazaar

howbazaar Oct 27, 2016

Owner

I've generally put these helper functions in serialization.go

mjs added some commits Oct 27, 2016

@mjs
core/description: Clean up optional times 
using schema.Omit and a new helper.
58a181f
@mjs
core/description: Update instance names 
... to match upstream changes.
6a38701
@mjs
Contributor

mjs commented Oct 27, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 27, 2016

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

jujubot commented Oct 27, 2016

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

mjs commented Oct 27, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 27, 2016

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

jujubot commented Oct 27, 2016

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

mjs commented Oct 27, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 27, 2016

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

@jujubot jujubot merged commit 2e0859c into juju:develop Oct 27, 2016

@mjs mjs deleted the mjs:MM-resources branch Oct 27, 2016

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