Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in no-docker and use-docker override to submit command #970

Merged
merged 3 commits into from
May 4, 2023
Merged

Add in no-docker and use-docker override to submit command #970

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3fade9
Add in no-docker and use-docker override to submit command
Jan 19, 2023
91f8a88
Remove reference to TypeScript
Jan 19, 2023
c5e38ea
Updated comment to be accurate about config file
Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/rpdk/core/submit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
def submit(args):
project = Project()
project.load()
# Use CLI override opposed to config file if use-docker or no-docker switch used
if args.use_docker or args.no_docker:
project.settings["use_docker"] = args.use_docker
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, feeding this CLI arg into project.settings will write the new flags to .rpdk-config.json and get picked up by future CLI commands. This may not be desirable if the user only wants to override this setting for a single run. Perhaps a better solution would be to extend project settings with a flag or something that lets you control whether the setting gets persisted or not.

Also, I notice that only the python plugin uses the use_docker setting today. The typescript plugin uses useDocker, and as far as I can tell neither the java or go plugins support building in a docker container at all. I think it's worth considering whether this behaviour can be standardised across all plugins (but that could be a future enhancement / PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript was updated to match the other two plugins a little while back and I tested this update on python and typescript extensions (hooks/resources) I don't have a golang extension handy but it should also apply.

Since config file actually only gets written during the "cfn init" portion of things. The override won't update the .rpdk-config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I'm basing this off the current master branch of the typescript plugin. The next release of the typescript plugin will have this use_docker update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops just noticed that comment, let me update that to be accurate :)

project.settings["no_docker"] = args.no_docker
project.submit(
args.dry_run,
args.endpoint_url,
Expand Down Expand Up @@ -51,3 +55,18 @@ def setup_subparser(subparsers, parents):
help="Register the type without an explicit execution role "
"(Will not be able to invoke AWS APIs).",
)

nodocker_group = parser.add_mutually_exclusive_group()
nodocker_group.add_argument(
"--use-docker",
action="store_true",
help="""Use docker for platform-independent packaging.
This is highly recommended unless you are experienced
with cross-platform packaging.""",
)
nodocker_group.add_argument(
"--no-docker",
action="store_true",
help="""Generally not recommended unless you are experienced
with cross-platform packaging.""",
)