stage.py review notes

1 - There are several hard-coded strings (stage names, paths, content-naming etc) which IME won't suit all users/environments
2 - Lots of trailing whitespace
3 - Lots of long lines which need breaking up
4 - Many comments spurious or non-descriptive (e.g function description comments and commented code)
5 - Mentions centos a lot ;)
6 - The dump_* functions appear to dump content to files - how does this differ (or improve on) the more generic features I recently contributed which dump content to JSON format files (activationkey_export, kickstart_export, configchannel_export)
7 - softwarechannel_diff : Why can't this be implemented as a generic "spacecmd softwarechannel_diff" command, so it's useful outside of the "stage" paradigm?
8 - Ditto softwarechannel_sync (and what is the benefit over just cloning the source channel into one named the same as the destination?)
9 - Same comments re configchannel_diff and configchannel_sync, kickstart_diff, activationkey_diff

Overall, I like some of the features (I was thinking of implementing some of the diff features myself, particularly for config-channels), but IMHO pushing the "stage" concept into spacecmd is a mistake - it pushes policy into a generic API wrapper utility, and it enforces naming conventions and process/workflow which will not suit all environments.  It also breaks the spacecmd "namespace" conventions where, for example, all kickstart related features are kickstart_* which will make usage less intutitive.

FWIW, almost all customers I've worked with have a requirement for this type of workflow (dev-preprod-prod) or some variation thereof, and the way I approach automating it is to have some simple bash wrapper scripts, which call spacecmd.  This approach separates the policy/process (which can easily change depedning on the environment and application) and the low-level implementation (e.g the XMLRPC API interaction, which is mostly common and reusable)

If you really want/need a pure-python implementation (and I can see the advantages), then I'd suggest contributing as much as possible of the common/reusable parts to spacecmd (as a number of small patches :), then using the spacecmd module as an import into your "high level" python process-automation code.

Hello Steve,

thank you for your remarks.

Am 20.02.2012 22:26, schrieb bugzilla:
> --- Comment #1 from Steve Hardy <shardy> 2012-02-20 16:26:17 EST ---
> stage.py review notes
> 
> 1 - There are several hard-coded strings (stage names, paths, content-naming
> etc) which IME won't suit all users/environments

thts true. 
While looking in the code of spacecmd I have seen support for using the config file /etc/spacecmd.conf, but because this config file did not exist on my system, I wasn't sure how to add own config stuff to it.
Instead, I started stage.py with a few definitions, that can be moved to a config file later.
Just tell me, what format would be acceptable.


> 2 - Lots of trailing whitespace

I'm not a very experienced using python and my editor did not show these whitespaces.
Removed in the newly attached version.

> 3 - Lots of long lines which need breaking up

That is true. Are there any tools that do this kind of formating automatically like perltidy?


> 4 - Many comments spurious or non-descriptive (e.g function description
> comments and commented code)

sorry for this.


> 5 - Mentions centos a lot ;)

As far as I can see, centos is only mentioned (a lot) in the example descriped in "do_stage_help".
To be honest, serios testing is only done with Spacewalk and CentOS.
However, if this is a problem, I can easily replace all occurences of "centos" with "rhel".


> 6 - The dump_* functions appear to dump content to files - how does this differ
> (or improve on) the more generic features I recently contributed which dump
> content to JSON format files (activationkey_export, kickstart_export,
> configchannel_export)

The staging dump files are mainly used to check, if spacewalk components like softwarechannel or kickstart profiles differ between stages.
For kickstart and activation keys they are based on do_*_details functions.
Advantage of this approach is, that the result of a diff is human readable.
I'm not aware, if proper diff functionality exists for JSON data.


> 7 - softwarechannel_diff : Why can't this be implemented as a generic "spacecmd
> softwarechannel_diff" command, so it's useful outside of the "stage" paradigm?
> 8 - Ditto softwarechannel_sync (and what is the benefit over just cloning the
> source channel into one named the same as the destination?)
> 9 - Same comments re configchannel_diff and configchannel_sync, kickstart_diff,
> activationkey_diff

it is already usable without the need to use the other staging stuff from the stage-modul.
However, right now, the function names all start with the naming prefix "stage_".

There are two reason, why I've implemented it in this way:
1. when my code will not be integrated, all my changes are in a seperated file/modul. This makes it easier for me to use current version of spacecmd together with my staging code. This reason vanishs, if my code or parts of the code gets integrated into spacecmd.
2. for the user, it is very convient to use the staging functions with only the source component. The target component is found automatically. Example:

using
  stage_softwarechannel_diff centos6-x86_64-dev-subchannel1
instead of
  stage_softwarechannel_diff centos6-x86_64-dev-subchannel1 centos6-x86_64-qas-subchannel1

The target channel if found automatically, so the user can not do mistakes when using spacecmd interactivly.
But the functionality to find the corresponding component is part of the staging code.


> Overall, I like some of the features (I was thinking of implementing some of
> the diff features myself, particularly for config-channels), but IMHO pushing
> the "stage" concept into spacecmd is a mistake - it pushes policy into a
> generic API wrapper utility, and it enforces naming conventions and
> process/workflow which will not suit all environments.  It also breaks the
> spacecmd "namespace" conventions where, for example, all kickstart related
> features are kickstart_* which will make usage less intutitive.
> 
> FWIW, almost all customers I've worked with have a requirement for this type of
> workflow (dev-preprod-prod) or some variation thereof, and the way I approach
> automating it is to have some simple bash wrapper scripts, which call spacecmd.
>  This approach separates the policy/process (which can easily change depedning
> on the environment and application) and the low-level implementation (e.g the
> XMLRPC API interaction, which is mostly common and reusable)

I see your point. However, I'm not aware of a simple shell based solution to handle this kind of scenarios. (Maybe you can send me an example, on how you have implemtented this?)

As you pointed out, "almost all customers [...] have a requirement for this type".
I've made the same experice and while looking for a solution to handle this,
I've found various approaches. It seams, when working with Spacewalk, everybody has his own set of scripts. At the mailing list, in the spacewalk wiki, from other people working with spacewalk I've found a lot of approaches, most of them not easily adaptable and/or outdated.
That also means, that everbody begining to use Spacewalk will have a hard start.
The best reusable code I've found so far is the spacecmd project.

I think, the Spacewalk project is lacking support for this widely used requirement.
I also think, that the best place to integrate it, would be inside the Spacewalk API (GUI?).
However, this if far out of my ability to implement it. The second best approach is an integration into spacecmd, because it is universal and have most of the required functionality already included.
Of course, I know, that the current approach is not flexible enough to fulfill all requirements. Especially, the naming conventions must be made configurable.
I produced this RFE to check, how the spacecmd community react.
If this kind of functionality is wanted by the spacecmd project, I'm willing to extend the code to make it usable for more scenarios.

I would like to see, if Spacewalk staging will be implemented by new users using the same basic framework in the future.

Created attachment 564885 [details]
support for staging, v2 (cleanup)

(In reply to comment #2)
> I see your point. However, I'm not aware of a simple shell based solution to
> handle this kind of scenarios. (Maybe you can send me an example, on how you
> have implemtented this?)

I finally found time to clean up my spacecmd wrapper scripts, get approval from my manager, and push them to github.  See:

https://github.com/hardys/clone_release_scripts

These scripts implement a fairly generic (and easy to modify) staging workflow, similar to yours, but without pushing any policy (e.g naming/paths) down into spacecmd.  It's definitely not perfect but there are plenty of examples of how to implement a "simple shell-based solution" on-top of spacecmd here FYI.

One of the main areas still TODO is the ability to "diff" config-channels, and complete "clone releases".  

I think you've already implemented something close to what I require here, so was hoping you might break down your patch and resubmit as I mentioned above.  If/when I find time, I will try to implement these features, possibly reusing some of your code.

Created attachment 580197 [details]
support for staging, v3

improvements

I've uploaded a new version of my staging code. It adds a few improvements, mostly for creating diffs of Activation-Keys and Config-Channel.

@Steve Hardy: i'll take a look at your code in the next few days to see, how some of my code might be used for this.

Hi Joerg, 

Thanks for the updated code.

I've quickly reviewed, and as you've not addressed most of my initial review comments, this is still not a candidate for merging into spacecmd (IMHO).

As mentioned previously, I'd be really happy if you could break down your patch into smaller chunks, and submit a number of separate, small patches, implementing the new features in the existing spacecmd command "namespaces"

e.g lets start with the diff ones, as I would certainly find those useful ;)

stage_softwarechannel_diff => softwarechannel_diff (in softwarechannel.py)
stage_configchannel_diff => configchannel_diff (in configchannel.py)
stage_kickstart_diff => kickstart_diff (in kickstart.py)
stage_activationkey_diff => activationkey_diff (in activationkey.py)

If you can submit clean patches implementing each of those features, with style and calling conventions similar to existing functions that would be really good, and I'm sure that Aron or myself would happily commit them.

If you don't indicate you're willing to do the above, I'll probably implement those features myself, and may well reuse some of your code.  

I have no idea when I may have time to do this though, and you'll lose credit since the commits will be from me, not you (as they will be if you submit patches via spacewalk-devel which are then committed)

See https://fedorahosted.org/spacewalk/wiki/PatchProcess for more information, please add me on Cc for any patches you submit.

Hi Steve,

I've started to break up my code into separate patches and hope I'll be able to submit something at the end of this week.

So we've taken the discussion to spacewalk-devel, and we've established we won't be using the patch in the form attached to this bz, so I'm closing this bug WONTFIX, and we can continue discussion of the individual features on spacewalk-devel.