Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metadata service continued #981

Merged
merged 21 commits into from
Oct 12, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
containermetadata: extract duplicated parsing code
adnxn committed Oct 4, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 508d9e53e898fd5e4aff39044c06e6d640dd15ce
30 changes: 19 additions & 11 deletions agent/containermetadata/manager.go
Original file line number Diff line number Diff line change
@@ -114,13 +114,7 @@ func (manager *metadataManager) Create(config *docker.Config, hostConfig *docker

// Acquire the metadata then write it in JSON format to the file
metadata := manager.parseMetadataAtContainerCreate(taskARN, containerName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of parsing, marshaling and writing the metadata file is duplicated between this and the Update method. Please extract that to a method of its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

data, err := json.MarshalIndent(metadata, "", "\t")
if err != nil {
return fmt.Errorf("create metadata for task %s container %s: %v", taskARN, containerName, err)
}

// Write the metadata to file
err = writeToMetadataFile(manager.osWrap, manager.ioutilWrap, data, taskARN, containerName, manager.dataDir)
err = manager.marshalAndWrite(metadata, taskARN, containerName)
if err != nil {
return err
}
@@ -148,12 +142,11 @@ func (manager *metadataManager) Update(dockerID string, taskARN string, containe

// Acquire the metadata then write it in JSON format to the file
metadata := manager.parseMetadata(dockerContainer, taskARN, containerName)
data, err := json.MarshalIndent(metadata, "", "\t")
err = manager.marshalAndWrite(metadata, taskARN, containerName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if you aren't doing anything with the error returned, you can just do return manager.marshalAndWrite(metadata, taskARN, containerName) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, fixed.

if err != nil {
return fmt.Errorf("update metadata for task %s container %s: %v", taskARN, containerName, err)
return err
}

return writeToMetadataFile(manager.osWrap, manager.ioutilWrap, data, taskARN, containerName, manager.dataDir)
return nil
}

// Clean removes the metadata files of all containers associated with a task
@@ -164,3 +157,18 @@ func (manager *metadataManager) Clean(taskARN string) error {
}
return manager.osWrap.RemoveAll(metadataPath)
}

func (manager *metadataManager) marshalAndWrite(metadata Metadata, taskARN string, containerName string) error {
data, err := json.MarshalIndent(metadata, "", "\t")
if err != nil {
return fmt.Errorf("create metadata for task %s container %s: %v", taskARN, containerName, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, needs more context: "create metadata for container %s in task %s: failed to marshal metadata"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

}

// Write the metadata to file
err = writeToMetadataFile(manager.osWrap, manager.ioutilWrap, data, taskARN, containerName, manager.dataDir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too, you could just do return writeMetadata...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, fixed.

if err != nil {
return err
}

return nil
}
3 changes: 2 additions & 1 deletion agent/containermetadata/write_metadata_unix.go
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@ import (

const (
mountPoint = "/opt/ecs/metadata"
tempFile = "temp_metadata_file"
)

// createBindsEnv will do the appropriate formatting to add a new mount in a container's HostConfig
@@ -51,7 +52,7 @@ func writeToMetadataFile(osWrap oswrapper.OS, ioutilWrap ioutilwrapper.IOUtil, d
}
metadataFileName := filepath.Join(metadataFileDir, metadataFile)

temp, err := ioutilWrap.TempFile(metadataFileDir, "temp_metadata_file")
temp, err := ioutilWrap.TempFile(metadataFileDir, tempFile)
if err != nil {
return err
}