Hi Austin,
I have reviewed you script. Generally we would ask for patch files that
we could apply, but in this case since the entire change is just the
addition of a file then simply providing the URL is fine.
In the future when doing command-line scripts in python you may want to
look into using the argparse (or optparse if it has to run on python <
2.7, which spacewalk scripts generally should be able to do since it we
build packages for RHEL 5) libraries for managing the command-line
arguments. They enable you to create and use the arguments you need in a
much more flexible, powerful, and user-friendly way. The decision of
wheither or not to use an external library is always an ongoing one for
a developer, a good developer will know when to use a library to solve
the problem at hand and which one to use. Of course that requires
knowing what is available, a task which is never complete. This is a
suggestion only, if you chose not to bother with optparse that for this
program that is fine.
Usage examples never go amiss, especially when it is not clear what an
particular argument is or what format it should be. For example the
server argument, should that be just hostname of the Spacewalk server?
In this particular case you were expecting the user to pass in something
like "https://$hostname/rpc/api"; to connect to Spacewalk's public API. A
Spacewalk developer might be expected to know that url, but a user of
the tool should not have to. You should update it to expect only the
hostname of the Spacewalk server (and if you had used a library like
argparse we could have even specified a default of 'localhost').
It is generally good practice to follow the style of other code in
whatever project you are contributing to. Python developers in
particular care a lot about code format, probably because spacing
matters in Python. When in doubt follow the python style guide.
http://www.python.org/dev/peps/pep-0008/ In paricular I'm refering to
the usual 4 spaces for indentation, where you use 8. It's not a huge
deal, but having code that looks the same from file to file aids in the
readability and ease of maintenance for the developers.
Finally, code should handle known problem cases whenever possible. You
mention in the comments, "The systems in the group MUST be different
from the target or else the sync will fail and the script exit." If
that's a known issue then why not catch the error if it occures and
continue syncing the rest of the systems? In this case the error
occuring means literally that there is nothing to do for that server, so
there is no harm in catching it and continuing on.
If you would be willing to make the changes mentioned in the last three
paragraphs I would be happy to commit it. Thanks!
-Stephen
On 12/02/2013 09:16 PM, Austin Lavinghouse wrote:
I just finished writing & testing a script that, given a group, will
sync every system in that group with a target system. This script was
written for my senior thesis. What's the proper way to contribute it
such that it can be added to the spacewalk/scripts directory?
You can view the script here: turing.cs.olemiss.edu/~aelaving/normalize.py
Any comments or critiques would be most appreciated.
Thanks,
A. E. Lavinghouse
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
