Alow specifying the os-morphing user script phase #99
+232
−65
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,42 +191,121 @@ def get_option_value_from_args(args, option_name, error_on_no_value=True): | |
| return value | ||
|
|
||
|
|
||
| def comma_separated_kv_to_dict(input_string: str) -> dict: | ||
|
Member
There was a problem hiding this comment. hmm, I wonder. cliff uses argsparse. Could we instead have something like: If it can, this may also simplify the handling before, we basically let cliff handle this for us. Source: https://docs.python.org/3/library/argparse.html#type
Member
Author
There was a problem hiding this comment. Thanks, I'll give it a try.
Member
Author
There was a problem hiding this comment. It works, it's just that the original error message gets discarded. For example: The resulting error message might be good enough though. |
||
| """Convert a comma separated list of key=value pairs to dict. | ||
|
|
||
| Example: some_key=some_val,some_other_key=some_other_val | ||
| -> {"some_key": "some_val", "some_other_key": "some_other_val"} | ||
| """ | ||
| out = {} | ||
| kv_pairs = input_string.split(",") | ||
| for kv_pair in kv_pairs: | ||
| try: | ||
| key, value = kv_pair.split("=") | ||
| except ValueError: | ||
| raise ValueError("Not a <key>=<value> pair: %s" % kv_pair) | ||
| out[key] = value | ||
| return out | ||
|
|
||
|
|
||
| def compose_user_scripts(global_scripts, instance_scripts): | ||
| ret = { | ||
| "global": {}, | ||
| "instances": {} | ||
| } | ||
| global_scripts = global_scripts or [] | ||
| instance_scripts = instance_scripts or [] | ||
| for global_script_str_params in global_scripts: | ||
| try: | ||
| params = comma_separated_kv_to_dict(global_script_str_params) | ||
| except ValueError: | ||
| raise ValueError( | ||
| "Invalid global user script parameter: %s. Expecting " | ||
| "<os_type>=<script_path>. Can optionally include a comma " | ||
| "separated phase parameter, " | ||
| "e.g. <os_type>=<script_path>,phase=<phase>" % | ||
| global_script_str_params) | ||
|
Dany9966 marked this conversation as resolved.
|
||
| phase = params.pop("phase", constants.PHASE_OSMORPHING_POST_OS_MOUNT) | ||
| if phase not in constants.USER_SCRIPT_PHASES: | ||
| raise ValueError( | ||
| f"Invalid user script phase: {phase}. " | ||
| "Available options are: " | ||
| f"{', '.join(constants.USER_SCRIPT_PHASES)}.") | ||
| if not params: | ||
| raise ValueError( | ||
| "OS type not specified. " | ||
| "Available options are: %s" % ", ".join(constants.OS_LIST)) | ||
| if len(params.keys()) > 1: | ||
| raise ValueError( | ||
| "Too many parameters. Expecting just the OS type.") | ||
|
Member
There was a problem hiding this comment. That's a bit confusing, considering we accept the phase too. |
||
| os_type = list(params.keys())[0] | ||
| script_path = params[os_type] | ||
| if os_type not in constants.OS_LIST: | ||
| raise ValueError( | ||
| "Invalid OS %s. Available options are: %s" % ( | ||
| os_type, ", ".join(constants.OS_LIST))) | ||
|
|
||
| payload = None | ||
| # The user may omit the script path in order to remove all script | ||
|
Contributor
There was a problem hiding this comment. Can you please mention this in the helper string of the command?
Member
Author
There was a problem hiding this comment. The behavior was already there but indeed, it should be documented. |
||
| # records. | ||
| if not script_path: | ||
| ret["global"][os_type] = None | ||
| continue | ||
|
|
||
| if not os.path.isfile(script_path): | ||
| raise ValueError("Could not find %s" % script_path) | ||
| with open(script_path) as sc: | ||
| payload = sc.read() | ||
| if os_type not in ret["global"]: | ||
| ret["global"][os_type] = [] | ||
| script_entry = { | ||
| "phase": phase, | ||
| "payload": payload, | ||
| } | ||
| ret["global"][os_type].append(script_entry) | ||
|
|
||
| for instance_scripts_str_params in instance_scripts: | ||
| try: | ||
| params = comma_separated_kv_to_dict(instance_scripts_str_params) | ||
| except ValueError: | ||
| raise ValueError( | ||
|
Dany9966 marked this conversation as resolved.
|
||
| "Invalid instance user script parameter: %s. Expecting " | ||
| "<instance>=<script_path>. Can optionally include a comma " | ||
| "separated phase parameter, " | ||
| "e.g. <instance>=<script_path>,phase=<phase>" % | ||
| instance_scripts_str_params) | ||
|
|
||
| phase = params.pop("phase", constants.PHASE_OSMORPHING_POST_OS_MOUNT) | ||
| if phase not in constants.USER_SCRIPT_PHASES: | ||
| raise ValueError( | ||
| f"Invalid user script phase: {phase}. " | ||
| "Available options are: " | ||
| f"{', '.join(constants.USER_SCRIPT_PHASES)}.") | ||
| if not params: | ||
| raise ValueError("Instance not specified.") | ||
| if len(params.keys()) > 1: | ||
| raise ValueError( | ||
| "Too many parameters. Expecting just one instance.") | ||
| instance = list(params.keys())[0] | ||
| script_path = params[instance] | ||
| payload = None | ||
| # The user may omit the script path in order to remove all script | ||
|
|
||
| # records. | ||
| if not script_path: | ||
| ret["instances"][instance] = None | ||
| continue | ||
|
|
||
| if not os.path.isfile(script_path): | ||
| raise ValueError("Could not find %s" % script_path) | ||
| with open(script_path) as sc: | ||
| payload = sc.read() | ||
| if instance not in ret["instances"]: | ||
| ret["instances"][instance] = [] | ||
| script_entry = { | ||
| "phase": phase, | ||
| "payload": payload, | ||
| } | ||
| ret["instances"][instance].append(script_entry) | ||
| return ret | ||
|
|
||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also mention what the phases are, and what the default is?