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.

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

feat!: show-config is now a command, slight refactor #74

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

AWS-Samuel
Copy link
Contributor

@AWS-Samuel AWS-Samuel commented Feb 15, 2024

What was the problem/requirement? (What/Why)

When implementing #73 I discovered that the entrypoint was a bit hard to grasp, and that adding new commands didn't fit very well. Also various bugs and documentation missing.

What was the solution? (How)

This PR accomplishes several things:

  • Reduces the length of the start() method in the entry point from 130 LOC to 27
  • Removes unnecessary __init__ implementations in Adaptor subclasses in tests
  • Fixes a bug in the IntegCommandAdaptor if the arguments passed in are not strings
  • Fixes a bug in the IntegManagerProcessed where get_arguments return a string instread of a list
  • Adds help text to some commands where it didn't exist prior
  • Changes the subcommands parser to be titled commands
  • <BREAKING> Changes the --show-config argument to it's own show-config command since it doesn't play nice with other commands anyway
  • Add handler for most commands to separate logic into digestible chunks
  • Reduce reliance on mocking _parse_args in unit tests, instead mock sys.argv so we can guarantee it works as expected, and the tests aren't brittle to adding a new command in the future.
  • <BREAKING> If no command is specified, an error is thrown instead of defaulting to the run command.

What is the impact of this change?

The help text is a bit more clear

Examples with the IntegCommandAdaptor

Old:

$  python -m IntegCommandAdaptor --help
usage: adaptor_runtime [-h] [--show-config] {run,daemon} ...

optional arguments:
  -h, --help     show this help message and exit
  --show-config  When specified, the adaptor runtime configuration is printed then the program exits.

subcommands:
  {run,daemon}

New:

$  python -m IntegCommandAdaptor --help
usage: IntegCommandAdaptor <command> [arguments]

optional arguments:
  -h, --help            show this help message and exit

commands:
  {show-config,run,daemon}
    show-config         Prints the adaptor runtime configuration, then the program exits.
    run                 Run through the start, run, stop, cleanup adaptor states.
    daemon              Runs the adaptor in a daemon mode.

The interface of the CLI has changed for --show-config.

Old:

$  python -m IntegCommandAdaptor --show-config 

New:

$  python -m IntegCommandAdaptor show-config

Adaptors no long run the run command when provided no command. This was broken anyway because init-data and run-data couldn't be passed in due to how we construct the argparser.

Old:

$ python -m IntegCommandAdaptor
INFO: Applying user-level configuration: /home/samcan/.openjd/worker/adaptors/runtime/configuration.json
INFO: Applying user-level configuration: /home/samcan/.openjd/adaptors/IntegCommandAdaptor/IntegCommandAdaptor.json
prerun-print
INFO: 
openjd_fail: Error encountered while running adaptor: can only concatenate list (not "str") to list
ERROR: Error running the adaptor: can only concatenate list (not "str") to list
Entrypoint failed: can only concatenate list (not "str") to list

New:

$ python -m IntegCommandAdaptor 
INFO: Applying user-level configuration: /home/samcan/.openjd/worker/adaptors/runtime/configuration.json
usage: IntegCommandAdaptor <command> [arguments]

optional arguments:
  -h, --help            show this help message and exit

commands:
  {show-config,run,daemon}
    show-config         Prints the adaptor runtime configuration, then the program exits.
    run                 Run through the start, run, stop, cleanup adaptor states.
    daemon              Runs the adaptor in a daemon mode.
usage: IntegCommandAdaptor <command> [arguments]
adaptor_runtime: error: No command was provided.

How was this change tested?

Manual testing depicted above, unit/integ tests pass.

Was this change documented?

?

Is this a breaking change?

Yes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AWS-Samuel AWS-Samuel requested a review from a team as a code owner February 15, 2024 23:07
@AWS-Samuel AWS-Samuel force-pushed the samuel/entrypoint-refactor branch 4 times, most recently from 71e4c0e to 3a2bc68 Compare February 16, 2024 16:30
ddneilson
ddneilson previously approved these changes Feb 16, 2024
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

This is great stuff; a huge improvement! Thanks for the contribution!

Signed-off-by: Samuel Anderson <119458760+AWS-Samuel@users.noreply.github.com>
@ddneilson ddneilson merged commit 8030890 into mainline Feb 16, 2024
9 checks passed
@ddneilson ddneilson deleted the samuel/entrypoint-refactor branch February 16, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants