Refactor Initialize to be easier to read, and update comments per review.

Signed-off-by: Ying Li <ying.li@docker.com>

client/client.go

@@ -143,18 +143,28 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		return err
 	}
 
-	// currently we only support server managing snapshots, and nothing else
+	// currently we only support server managing timestamps and snapshots, and
-	rolesAreManaged := map[string]bool{
+	// nothing else - timestamps are always managed by the server, and implicit
-		data.CanonicalTimestampRole: true,
+	// (do not have to be passed in as part of `serverManagedRoles`, so that
-		data.CanonicalSnapshotRole:  false,
+	// the API of Initialize doens't change).
-		data.CanonicalTargetsRole:   false,
+	var serverManagesSnapshot bool
+	locallyManagedKeys := []string{
+		data.CanonicalTargetsRole,
+		data.CanonicalSnapshotRole,
+		// root is also locally managed, but that should have been created
+		// already
 	}
+	remotelyManagedKeys := []string{data.CanonicalTimestampRole}
 	for _, role := range serverManagedRoles {
 		switch role {
 		case data.CanonicalTimestampRole:
-			continue // always support timestamp
+			continue // timestamp is already in the right place
 		case data.CanonicalSnapshotRole:
-			rolesAreManaged[data.CanonicalSnapshotRole] = true
+			// because we put Snapshot last
+			locallyManagedKeys = []string{data.CanonicalTargetsRole}
+			remotelyManagedKeys = append(
+				remotelyManagedKeys, data.CanonicalSnapshotRole)
+			serverManagesSnapshot = true
 		default:
 			return fmt.Errorf(
 				"Notary does not support the server managing the %s key", role)
@@ -194,33 +204,28 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 
 	// we want to create all the local keys first so we don't have to
 	// make unnecessary network calls
-	for _, isManaged := range []bool{false, true} {
+	for _, role := range locallyManagedKeys {
-		for role, shouldBeManaged := range rolesAreManaged {
+		// This is currently hardcoding the keys to ECDSA.
-			if isManaged != shouldBeManaged {
+		key, err := r.CryptoService.Create(role, data.ECDSAKey)
-				continue
+		if err != nil {
+			return err
 		}
-			var key data.PublicKey
+		if err := addKeyForRole(kdb, role, key); err != nil {
-			if isManaged {
+			return err
+		}
+	}
+	for _, role := range remotelyManagedKeys {
 		// This key is generated by the remote server.
-				key, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
+		key, err := getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
 		if err != nil {
 			return err
 		}
 		logrus.Debugf("got remote %s %s key with keyID: %s",
 			role, key.Algorithm(), key.ID())
-			} else {
+		if err := addKeyForRole(kdb, role, key); err != nil {
-				// This is currently hardcoding the keys to ECDSA.
-				key, err = r.CryptoService.Create(role, data.ECDSAKey)
-				if err != nil {
 			return err
 		}
 	}
-			err = addKeyForRole(kdb, role, key)
-			if err != nil {
-				return err
-			}
-		}
-	}
 
 	r.tufRepo = tuf.NewRepo(kdb, r.CryptoService)
 
@@ -240,7 +245,7 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		return err
 	}
 
-	return r.saveMetadata(rolesAreManaged[data.CanonicalSnapshotRole])
+	return r.saveMetadata(serverManagesSnapshot)
 }
 
 // AddTarget adds a new target to the repository, forcing a timestamps check from TUF
@@ -346,7 +351,6 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) {
 // Conceptually it performs an operation similar to a `git rebase`
 func (r *NotaryRepository) Publish() error {
 	var updateRoot bool
-	var root *data.Signed
 	// attempt to initialize the repo from the remote store
 	c, err := r.bootstrapClient()
 	if err != nil {
@@ -357,6 +361,8 @@ func (r *NotaryRepository) Publish() error {
 			if err != nil {
 				// There are lots of reasons there might be an error, such as
 				// corrupt metadata.  We need better errors from bootstrapRepo.
+				logrus.Debugf("Unable to load repository from local files: %s",
+					err.Error())
 				return err
 			}
 			// We had local data but the server doesn't know about the repo yet,
@@ -395,25 +401,23 @@ func (r *NotaryRepository) Publish() error {
 
 	// these are the tuf files we will need to update, serialized as JSON before
 	// we send anything to remote
-	update := make(map[string][]byte)
+	updatedFiles := make(map[string][]byte)
 
 	// check if our root file is nearing expiry. Resign if it is.
 	if nearExpiry(r.tufRepo.Root) || r.tufRepo.Root.Dirty || updateRoot {
-		root, err = r.tufRepo.SignRoot(data.DefaultExpires("root"))
+		rootJSON, err := serializeCanonicalRole(r.tufRepo, data.CanonicalRootRole)
-
-		rootJSON, err := json.Marshal(root)
 		if err != nil {
 			return err
 		}
-		update[data.CanonicalRootRole] = rootJSON
+		updatedFiles[data.CanonicalRootRole] = rootJSON
 	}
 
-	// we will always resign targets, and snapshots if we have the snapshots key
+	// we will always re-sign targets
 	targetsJSON, err := serializeCanonicalRole(r.tufRepo, data.CanonicalTargetsRole)
 	if err != nil {
 		return err
 	}
-	update[data.CanonicalTargetsRole] = targetsJSON
+	updatedFiles[data.CanonicalTargetsRole] = targetsJSON
 
 	// do not update the snapshot role if we do not have the snapshot key or
 	// any snapshot data.  There might not be any snapshot data the repo was
@@ -425,11 +429,12 @@ func (r *NotaryRepository) Publish() error {
 		snapshotJSON, err := serializeCanonicalRole(
 			r.tufRepo, data.CanonicalSnapshotRole)
 		if err == nil { // we have the key - snapshot signed, let's update it
-			update[data.CanonicalSnapshotRole] = snapshotJSON
+			updatedFiles[data.CanonicalSnapshotRole] = snapshotJSON
 		} else if _, ok := err.(signed.ErrNoKeys); ok {
 			logrus.Debugf("Client does not have the key to sign snapshot. " +
 				"Assuming that server should sign the snapshot.")
 		} else {
+			logrus.Debugf("Client was unable to sign the snapshot: %s", err.Error())
 			return err
 		}
 	}
@@ -439,7 +444,7 @@ func (r *NotaryRepository) Publish() error {
 		return err
 	}
 
-	err = remote.SetMultiMeta(update)
+	err = remote.SetMultiMeta(updatedFiles)
 	if err != nil {
 		return err
 	}