Skip to content

Fix for issue #383 & Support for immediate cancellation of servers#384

Merged
sudorandom merged 2 commits into
softlayer:masterfrom
wnagele:master
Sep 18, 2014
Merged

Fix for issue #383 & Support for immediate cancellation of servers#384
sudorandom merged 2 commits into
softlayer:masterfrom
wnagele:master

Conversation

@wnagele
Copy link
Copy Markdown
Contributor

@wnagele wnagele commented Sep 4, 2014

No description provided.

@sudorandom
Copy link
Copy Markdown
Contributor

Thanks for the pull request.

I'm looking into why the CI failed. It's not related to what you've done. I expect to post a pull request to clear that up shortly.

@sudorandom
Copy link
Copy Markdown
Contributor

I was wrong. 2 out of the 3 failures were due to other issues (being addressed in #385) but one of the tests is failing 'correctly': https://travis-ci.org/softlayer/softlayer-python/jobs/34384950#L761

@wnagele wnagele changed the title Fix for issue #383 Fix for issue #383 & Support for immediate cancellation of servers Sep 5, 2014
@wnagele
Copy link
Copy Markdown
Contributor Author

wnagele commented Sep 5, 2014

Should be good now.

Comment thread SoftLayer/CLI/modules/server.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sure this fits with our "style guide" for CLI options: http://softlayer-python.readthedocs.org/en/latest/dev/cli.html#defining-a-module

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I virtually just copied this from here: https://github.com/softlayer/softlayer-python/blob/6f5c229c28ce8466c961aec1bcb253e88875bc21/SoftLayer/CLI/modules/iscsi.py

Anything in particular that's not right about it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's correct in isolation, but the description doesn't line up with the other descriptions below it. Here's how it should look:

  --immediate        Cancels the server immediately (instead of on the billing
                       anniversary)
  --comment=COMMENT  An optional comment to add to the cancellation ticket
  --reason=REASON    An optional cancellation reason. See cancel-reasons for a
                       list of available options

If that doesn't make sense it's not a big deal. I can fix it up later if you want.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. It's pretty now. ;)

@sudorandom
Copy link
Copy Markdown
Contributor

To avoid pull requests from being tied additional changes that you're working on, it's typical to create a new branch on your fork and use that to submit the pull request. This might help make this a bit clearer: https://guides.github.com/introduction/flow/index.html

@sudorandom
Copy link
Copy Markdown
Contributor

+1

sudorandom added a commit that referenced this pull request Sep 18, 2014
Fix for issue #383 & Support for immediate cancellation of servers
@sudorandom sudorandom merged commit d21a6cd into softlayer:master Sep 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants