Add shell scripts to export/import alerts and alert profiles #112
Conversation
branic
changed the title
branic
changed the title
|
@carbonin Please review. |
|
@branic I'm trying to understand the purpose of these shell scripts over just calling the rake task itself. Is it just for a pretty interface or is there some deeper reason? |
There was a problem hiding this comment.
I'm going to leave it at this as a first pass.
Also, agree with @Fryguy. It seems like it would be easier to just have people call the rake tasks directly.
Then we have less code to maintain (whenever anyone adds a new export task, they would also need to know to update these scripts) and the functionality can be rolled into automated testing (which we don't do here).
LINK/usr/bin/evmexport
Outdated
| } | ||
|
|
||
| usage () { | ||
| progname=`basename $0` |
There was a problem hiding this comment.
I think this should be indented.
LINK/usr/bin/evmexport
Outdated
| return 1 | ||
| } | ||
|
|
||
| parse_arguments () { |
There was a problem hiding this comment.
I think I would rather not have a function for this.
- I usually expect option parsing to be in the main script and
- I know it works, but it feels odd to be setting
remaining_argsin a function and then using it after.
There was a problem hiding this comment.
I'll move this out of the function into the main-line code.
LINK/usr/bin/evmexport
Outdated
| KEEP_SPACES="true" | ||
| shift # past the argument | ||
| ;; | ||
| --) |
There was a problem hiding this comment.
What is the use case for having this? It feels unnecessary as we already stop parsing when we see the first thing that isn't -s|--keep-spaces.
There was a problem hiding this comment.
This is for backwards compatibility. In previous versions of the rake tasks the files created had spaces in the file names.
I would be comfortable removing this option in ManageIQ as the namespaces of the rake tasks will be different (since the rhconsulting namespace needs to be changed).
LINK/usr/bin/evmexport
Outdated
| break # stop parsing arguments | ||
| ;; | ||
| *) | ||
| # unknown option |
There was a problem hiding this comment.
All the comments in this case block feel unnecessary to me.
LINK/usr/bin/evmexport
Outdated
| esac | ||
| done | ||
|
|
||
| # if there are any required arguments check for them here |
There was a problem hiding this comment.
I think we can remove this comment.
LINK/usr/bin/evmexport
Outdated
|
|
||
| # if there are any required arguments check for them here | ||
|
|
||
| # pass the shifted arguments back to the caller |
There was a problem hiding this comment.
I think this is not needed if this logic is not in a separate function. Setting remaining_args becomes less confusing if it is done right before it is used.
LINK/usr/bin/evmexport
Outdated
| } | ||
|
|
||
| parse_action () { | ||
| AVAILABLE_PARSERS=`compgen -A function | grep '^op_'` |
There was a problem hiding this comment.
I think this is just an opportunity for weird bugs. I would rather be a bit more verbose and just validate against a known set of options.
There was a problem hiding this comment.
I'll specify a fixed list for this.
LINK/usr/bin/evmexport
Outdated
|
|
||
| obj_type="$1" | ||
| op_func="op_${obj_type}" | ||
| contains $op_func $AVAILABLE_PARSERS; VALID_OP=$? |
There was a problem hiding this comment.
I think we can avoid all of this logic if we just make the object type a proper argument to the script something like --object-type|-o
Then if we don't match we can just fail there.
There was a problem hiding this comment.
Object type is a positional argument to the script and is required. I can make it so that it has to be passed in the option format instead of a positional argument, but there still needs to be some way to check that the type of object the user wants to export is valid (i.e. alerts is a valid export object type, but alert isn't) which is what this line is doing.
There was a problem hiding this comment.
If the object type argument is changed to the option format then the directory argument should also be changed to the option format so there are no positional arguments, just option arguments.
|
The shell scripts do make for a prettier interface, but are meant to make it easier for a system administrator to use. Many system administrators aren't familiar with running rake tasks, but are familiar with running shell scripts. In addition to the above they also provide an easier way to pass in arguments for the rake tasks from the command line. |
|
Checked commits https://github.com/branic/manageiq-appliance/compare/7511ba25341c52641b57dc5fe2b913761c75f3f8~...b889874c74dcb2a822b314b2cf0a869388a4b5a6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
This pull request has been automatically closed because it has not been updated for at least 6 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! |
These two scripts are used to export/import various ManageIQ objects. They are wrapper scripts for rake scripts that actually perform the export/import.
These scripts are from the CFME RH Consulting Scripts and are used by Red Hat consultants to enable storing customizations in Git and maintaining customizations between environments (e.g. dev/qa/prod) for an SDLC lifecycle.
PR 14126 in manageiq is tied to this PR and provides the rake scripts that these bash scripts call.