Support CLI arguments for cfn init #574
Conversation
There was a problem hiding this comment.
The only thing that I am hesitant about this approach is that running cfn init --help will produce a lot very verbose set all arguments used across plugins. I know we kind of went over this previously, but maybe it is possible to add a unique parser for each plugin, then surface this using the plugin registry?
Though this may just be something we look at down the road. Other than that the current approach looks good, just want someone else to take a look at it.
src/rpdk/core/init.py
Outdated
| from functools import wraps | ||
|
|
||
| from colorama import Fore, Style | ||
|
|
||
| from .exceptions import WizardAbortError, WizardValidationError | ||
| from .plugin_registry import PLUGIN_CHOICES | ||
| from .plugin_registry import get_plugin_choices |
There was a problem hiding this comment.
any reason why we are adding this method in? seems to be the same value as PLUGIN_CHOICES
There was a problem hiding this comment.
For testing we add a dummy plugin on the fly but at that point in time PLUGIN_CHOICES is already a constant that does not includes it, making it a method allows to get the latest installed plugins.
src/rpdk/core/init.py
Outdated
| print( | ||
| Style.BRIGHT, | ||
| Fore.RED, | ||
| "The plugin for {} is not installed.".format(language), |
There was a problem hiding this comment.
| "The plugin for {} is not installed.".format(language), | |
| "The plugin for {} cannot be found.".format(language), |
There was a problem hiding this comment.
we probably should not always print colorama otherwise it will be really hard to maintain;
maybe we can use for an abstract exception which all will change color entry
src/rpdk/core/init.py
Outdated
| { | ||
| "use_docker": args.use_docker, | ||
| "namespace": args.namespace, | ||
| "codegen_template_path": args.codegen_model, |
There was a problem hiding this comment.
codegen-model is specific to java plugin and not supported by any other ones; will this fail project init if it's provided? if not we should and assert that it's not supported by the plugin;
There was a problem hiding this comment.
At the moment, each plugin looks for the arguments it needs in project.settings, the fact that additional un-related information is also coming in does not affects the plugin. We sure can do some validation and only allow arguments related to the selected plugin but I don't know if is worth doing that validation or how it will impact the user experience, if the user passes a wrong flag s/he would need to try again (kind of defeating the purpose of being able to use cfn init as just a one-liner). What do you think?
src/rpdk/core/init.py
Outdated
| try: | ||
| type_name = validate_type_name(args.type_name) | ||
| except WizardValidationError as error: | ||
| print(Style.BRIGHT, Fore.RED, str(error), Style.RESET_ALL, sep="") |
There was a problem hiding this comment.
can we do it a bit more generic and reusable?
src/rpdk/core/init.py
Outdated
| print( | ||
| Style.BRIGHT, | ||
| Fore.RED, | ||
| "The plugin for {} is not installed.".format(language), |
There was a problem hiding this comment.
we probably should not always print colorama otherwise it will be really hard to maintain;
maybe we can use for an abstract exception which all will change color entry
src/rpdk/core/init.py
Outdated
| "-f", | ||
| "--force", | ||
| action="store_true", | ||
| help="Force files to be overwritten.", | ||
| ) | ||
|
|
||
| parser.add_argument( | ||
| "-l", | ||
| "--language", | ||
| help="""Select a language for code generation. | ||
| The language plugin needs to be installed.""", | ||
| ) | ||
|
|
||
| parser.add_argument( | ||
| "-t", | ||
| "--type-name", | ||
| help="Select the name of the resource type.", | ||
| ) | ||
|
|
||
| parser.add_argument( | ||
| "-d", | ||
| "--use-docker", | ||
| action="store_true", | ||
| help="""Use docker for python platform-independent packaging. | ||
| This is highly recommended unless you are experienced | ||
| with cross-platform Python packaging.""", | ||
| ) | ||
|
|
||
| parser.add_argument( | ||
| "-n", | ||
| "--namespace", |
There was a problem hiding this comment.
@jotompki do you think we could use something like a config file to specify all the metadata - ConfigArgParse is one of the options
| plugin_choices = [ | ||
| entry_point.name | ||
| for entry_point in pkg_resources.iter_entry_points("rpdk.v1.languages") | ||
| ] |
There was a problem hiding this comment.
I would use set if you dont rely on element position
There was a problem hiding this comment.
Before my changes the list of plugins was already being sorted which converts the set back to a list. I f this is not required anymore I'll be ok with making it a set.
tests/test_init.py
Outdated
| args = get_args() | ||
| mock_project, patch_project = get_mock_project() | ||
|
|
||
| patch_tn = patch("rpdk.core.init.input_typename") |
There was a problem hiding this comment.
do these need to be patched if we are going the non-interactive route?
tests/test_init.py
Outdated
| mock_project, patch_project = get_mock_project() | ||
|
|
||
| patch_tn = patch("rpdk.core.init.input_typename") | ||
| patch_l = patch("rpdk.core.init.input_language") |
There was a problem hiding this comment.
same here. no reason to patch if we are just asserting we aren't calling
tests/utils.py
Outdated
| None if interactive else ("dummy" if language else "invalid_language") | ||
| ) | ||
| args.type_name = ( | ||
| None if interactive else ("Test::Test::Test" if type_name else "Test") |
There was a problem hiding this comment.
let's just pass in a type name into this get_args util instead of passing in boolean that sets between two values. makes the function more straightforward
tests/utils.py
Outdated
| ) | ||
|
|
||
| args.language = ( | ||
| None if interactive else ("dummy" if language else "invalid_language") |
There was a problem hiding this comment.
same here as below. no need to obfuscate what is going on
tests/test_init.py
Outdated
| mock_project.init.assert_called_once_with( | ||
| args.type_name, | ||
| args.language, | ||
| args.settings, |
There was a problem hiding this comment.
usually args doesn't have a settings member, correct? let's keep args as simple as possible and in line with what the cli expects its structure to be. it looks like here it is only used to keep track of test values. can always pull that logic out of the get_args method and inline it here.
tests/test_init.py
Outdated
| mock_project.load_settings.side_effect = FileNotFoundError | ||
| mock_project.settings_path = "" | ||
| mock_project.root = Path(".") | ||
| args = get_args(interactive=True) |
There was a problem hiding this comment.
is this even being used in this test other than the settings?
| mock_project.generate.assert_called_once_with() | ||
|
|
||
|
|
||
| def test_init_method_noninteractive_invalid_type_name(): |
There was a problem hiding this comment.
confused on this. how does init reach project init method if the typename is invalid?
Issue #, if available:
#410
Description of changes:
Add the ability to pass as CLI arguments all the required values to create projects on all the three public language plugins. These new arguments are:
Ideally this PR should be merged (or at least the changes. released) after #121, #309 and #168.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.