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 methods to get and set the glusterfs op-version #45091

Conversation

robertpenberthy
Copy link
Contributor

What does this PR do?

Adds methods to the existing glusterfs execution and state modules to querying and setting the glusterfs op-version.

What issues does this PR fix or reference?

None

Tests written?

No

Commits signed with GPG?

Yes

@robertpenberthy
Copy link
Contributor Author

This is my first PR to this project and Github, so please let me know if there is anything I missed / could do better.

I made these changes locally on my salt master to assist with upgrading Glusterfs versions in our environment. Post upgrade, glusterfs documentation recommends updating the op-version, http://docs.gluster.org/en/latest/Upgrade-Guide/op_version/. Now I'm just trying to share my additions with the community.

@robertpenberthy
Copy link
Contributor Author

Also, according to the coding-style section, I need to bring up that I am not sure what should be added to the function documentation for versionadded.

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Welcome @robertpenberthy! Thank you for submitting your addition. This is a really great start.

I have a couple of comments. Most are documentation updates, but there are a couple of things in the state functions that need to be fixed in the code.

Can you also write some tests for these new functions?

Please let us know if you have any questions!


def get_op_version(name):
'''
Returns the glusterfs volume op-version
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a .. versionadded:: Fluorine tag here for this new function?

This comment applies to the other functions you've added here as well. :)

'''

version = ''
cmd = 'gluster volume get {0} cluster.op-version'.format(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to a list of command options, rather than a string? It's safer that way since name could pass in anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure I understand what you're asking, is this what I should change it to?

cmd = ['gluster', 'volume', 'get', name, 'cluster.op-version']

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's it exactly!

salt '*' glusterfs.set_op_version <volume>
'''

cmd = 'gluster volume set all cluster.op-version {0}'.format(version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about lists vs. a string.

def op_version(name, version):
'''
Add brick(s) to an existing volume

Copy link
Contributor

Choose a reason for hiding this comment

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

versionadded tags are needed for these 2 new state functions as well.

'''
Returns the glusterfs volume's max op-version value
Requires Glusterfs version > 3.9
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

This function needs a CLI Example.

ret['result'] = True
return ret

result = __salt__['glusterfs.set_op_version'](version)
Copy link
Contributor

Choose a reason for hiding this comment

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

This state function needs to support test=true behavior before actually changing anything. Can you add that, please?

ret['result'] = True
return ret

result = __salt__['glusterfs.set_op_version'](max_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

This state function also needs to support test=true functionality.

@rallytime rallytime added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 21, 2017
robertpenberthy and others added 2 commits December 22, 2017 14:22
Changing cmd to list.
Adding test=True handling for the states.
Added try / except around calls to the execution module.
@rallytime
Copy link
Contributor

Thanks @robertpenberthy! This looks great. Now all we need is some unit tests and we'll be good to go. There's some docs on that here. Let us know if you have any questions. :)

@robertpenberthy
Copy link
Contributor Author

@rallytime Yep, I'm aware that I need to add those unit tests. It may take me a while to get those done, due to some of my other priorities, as well as I am new to Python (doing this custom module is pretty much the only Python I have ever written) and have never written Unit tests before. I hope to get through the documentation soon and then the tests written. I've looked at the existing tests for these modules and hopefully can easily pattern after them.

@rallytime
Copy link
Contributor

@robertpenberthy Wonderful! Thank you so much. Definitely let us know if you have troubles.

@robertpenberthy
Copy link
Contributor Author

@rallytime So I've been looking at how the tests are written for the existing functions in the module execution module and realize that these methods are all executing their commands through the _gluster_xml private method. I've also learned a bit more about how the existing functions work.

The existing functions use the string.format(variable) syntax, which you told me to change to a list for my functions. When passing the list to the _gluster_xml function, it returns with an error. It does work if I use the string.format(variable) syntax.

So now I am wondering, should I switch my command back to string.format(variable) and make my functions execute the commands in the same way as the existing code? Should I go through and change the existing functions to use and accept the list format? Or break from the existing pattern in this specific module and its unit tests, executing my code how I wrote it initially and write my tests to fit how I executed the commands?

robertpenberthy and others added 2 commits December 28, 2017 18:20
- Changed the functions to execute the commands in the same manner as the pre-existing functions.
- Added unit tests
@robertpenberthy
Copy link
Contributor Author

@rallytime I assumed I should change it up to be like the pre-existing fucntions and did so, then wrote some unit tests. Just wrote them for the execution module functions so far. Please let me know if I got anything wrong with them.

@robertpenberthy
Copy link
Contributor Author

@rallytime I've added tests. I believe I've completed all requests in the code review.

@rallytime rallytime removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jan 18, 2018
@rallytime
Copy link
Contributor

Thanks @robertpenberthy!

@rallytime rallytime merged commit 2908a35 into saltstack:develop Jan 18, 2018
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.

None yet

2 participants