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

Add module for Pure Storage FlashBlade array #45993

Merged
merged 1 commit into from Feb 26, 2018

Conversation

Projects
None yet
2 participants
@sdodsley
Copy link
Contributor

commented Feb 12, 2018

What does this PR do?

Provides ability to manage filesystems and snapshots on Pure Storage FlashBlade external storage arrays

Tests written?

Yes

Commits signed with GPG?

No

@sdodsley sdodsley force-pushed the sdodsley:purefb_module branch 3 times, most recently from d162dba to fd4f14b Feb 13, 2018

@rallytime
Copy link
Contributor

left a comment

@sdodsley This looks great. Thanks for submitting this. I have some requested changes, mostly for getting this to match salt-style.

If you could also add some unit tests for this new module, that would be fantastic.

:requires: purestorage
:platform: all
.. versionadded:: Oxygen

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

This should be updated to Fluorine :)

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

I see some other references to Oxygen in this file as well, but those should all be Fluorine. If you could update those, too, that'd be great!


__docformat__ = 'restructuredtext en'

VERSION = '1.0.0'

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

Can you remove these global variables here (VERSION and USER_AGENT_BASE)? I don't see these used any where in this file.



def _get_fs(name, blade):
'''Private function to check filesystem'''

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

Can you expand this out to all separate lines? I know that's picky, but Salt style is to not have docs on one line like this.



def _get_snapshot(name, suffix, blade):
"""Return Snapshot or None"""

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

Same comment here about one-line docs. Can you also change these double quotes to single quotes? Salt style calls for single quoted doc-strings, rather than double quotes.



def _get_deleted_fs(name, blade):
'''Private function to check deleted filesystem'''

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

Same comment here ;)

def _get_snapshot(name, suffix, blade):
"""Return Snapshot or None"""
try:
filt = 'source=\'' + name + '\' and suffix=\'' + suffix + '\''

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

I think this would be a lot easier to read with .format() here.

Create a filesystem snapshot on a Pure Storage FlashBlade.
Will return False is filesystem selected to snap does not exist.

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

is --> if

print(proto)
if _get_fs(name, blade) is None:
if size is None:
size = __utils__['stringutils.human_to_bytes']('32G')

This comment has been minimized.

Copy link
@rallytime

rallytime Feb 15, 2018

Contributor

Nice use of __utils__ 👍

@sdodsley sdodsley force-pushed the sdodsley:purefb_module branch from fd4f14b to de94a24 Feb 15, 2018

@sdodsley sdodsley changed the title Add module for Pure Storage FlashBlade array WIP - Add module for Pure Storage FlashBlade array Feb 15, 2018

@sdodsley sdodsley force-pushed the sdodsley:purefb_module branch from de94a24 to 4a5718e Feb 17, 2018

@sdodsley sdodsley changed the title WIP - Add module for Pure Storage FlashBlade array Add module for Pure Storage FlashBlade array Feb 22, 2018

@sdodsley sdodsley force-pushed the sdodsley:purefb_module branch from 74a7ef8 to 333b5af Feb 22, 2018

@sdodsley

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

GO GO Jenkins!

@sdodsley

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

@rallytime will this do? Why are all checks failing????

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

Thanks for adding those tests @sdodsley! Yeah, those test failures are unrelated. We are working on getting those fixed up. However, there are some related lint failures: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/19496/violations/file/tests/unit/modules/test_purefb.py/

Can you fix those?

@sdodsley sdodsley force-pushed the sdodsley:purefb_module branch from 333b5af to b0403be Feb 24, 2018

@sdodsley

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

@rallytime fixed the lint failures. Sorry about that - they got lost in the noise of the other failures 😊

@rallytime rallytime merged commit 87ba05c into saltstack:develop Feb 26, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #2539 — ABORTED
Details
codeclimate 14 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #20184 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #7102 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #16590 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #22584 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #14928 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #19539 — SUCCESS
Details
@sdodsley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

@rallytime can you force a rerun of the webpage creation for the develop execution modules documentation so this module becomes visible

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

@sdodsley I should have caught this in the review, but you'll need to add some doc directives for sphinx to pick this up and build it automatically in the docs. Can you add a PR for this?

See the examples in doc/ref/modules/all/* for how to do this. A file for the new module is needed as well as an entry in the index.rst file.

@sdodsley sdodsley deleted the sdodsley:purefb_module branch Mar 17, 2018

@sdodsley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

@rallytime PR #46590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.