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

Volume Attachment States #319

Merged

Conversation

akutz
Copy link
Collaborator

@akutz akutz commented Nov 4, 2016

This patch adds the AttachmentState field to the Volume object model to more summarily reflect a volume's attachment state.

@akutz akutz self-assigned this Nov 4, 2016
This patch adds the AttachmentState field to the Volume object model to
more summarily reflect a volume's attachment state.
@akutz akutz force-pushed the feature/volume-attachment-states branch from 35b21f8 to 34ffe54 Compare November 4, 2016 15:38
@codecov-io
Copy link

Current coverage is 25.01% (diff: 100%)

Merging #319 into release/0.3.3 will not change coverage

@@           release/0.3.3       #319   diff @@
===============================================
  Files                 47         47          
  Lines               2830       2830          
  Methods                0          0          
  Messages               0          0          
  Branches               0          0          
===============================================
  Hits                 708        708          
  Misses              2052       2052          
  Partials              70         70          

Powered by Codecov. Last update 243107c...34ffe54

@akutz akutz merged commit b4fb6fd into thecodeteam:release/0.3.3 Nov 4, 2016
@akutz akutz deleted the feature/volume-attachment-states branch November 4, 2016 16:01
@akutz akutz removed the in progress label Nov 4, 2016
Copy link
Contributor

@codenrhoden codenrhoden left a comment

Choose a reason for hiding this comment

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

I know this is already merged, but a couple simple questions for you...

if v.AttachmentState == 0 {
v.AttachmentState = types.VolumeAttached
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this one 0 -> attached and all the others 0 -> available? My guess is that this is basically setting the default value, and if a call to Attach() is returning without an error, it can be assumed that the volume is attached. makes sense, but want to double check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You inferred the reasoning correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although now that you point this out, I need to update the above to the following logic:

if v.AttachmentState == 0 && len(v.Attachments) > 0 {
    if "attached to the same instance as the instance ID in the context" {
        v.AttachmentState = types.VolumeAttached
    } else {
        v.AttachmentState = types.VolumeUnavailable
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait, no I won't. The API call asserts the instance ID must be included to do the attach op, which means when the volume returns the volume WILL be attached (if successful) to whichever instance was specified in the call.

@@ -139,28 +169,28 @@ type Volume struct {
IOPS int64 `json:"iops,omitempty" yaml:"iops,omitempty"`

// The name of the volume.
Name string `json:"name"`
Name string `json:"name" yaml:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is YAML omitempty, but not JSON? Seems like the only outlier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, because I got Copy/Paste happy with the YAML update. Doesn't matter ultimately because the YAML tags were originally there to influence the CLI output which is no longer based on YAML.

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

Successfully merging this pull request may close these issues.

3 participants