Skip to content

commands: a variety of small cli handling cleanups #153

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

Merged
merged 13 commits into from
May 16, 2025

Conversation

phlogistonjohn
Copy link
Collaborator

In preparation for some future work, clean up how we construct the sambacc command line tools: samba-container and samba-dc-container. Make common stuff common, add docstrings and make things less special.

@phlogistonjohn phlogistonjohn marked this pull request as ready for review May 7, 2025 17:36
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Can we use commands.include() or commands.include_multiple() for those imports with noqa comments in initialize.py?

@phlogistonjohn
Copy link
Collaborator Author

Can we use commands.include() or commands.include_multiple() for those imports with noqa comments in initialize.py?

Not directly as I added the new method to handle the functions that are decorated by commands decorator. The imports in initialize.py are there to pull in the setup step decorators. We could add something similar in the future for that but I think I would not want to do that for this pr.

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@phlogistonjohn
Copy link
Collaborator Author

@Mergifyio rebase

When a docstring is present the ellipsis to create a function body is
not necessary. Remove them in cli.py

Signed-off-by: John Mulligan <jmulligan@redhat.com>
The cli.ceph_id is a copy of the main._ceph_id function moved to
cli.py as it is a option parsing type that could be reused elsewhere
in the future.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
The samba-container command that is based on main.py shares a bunch of
functions with dcmain.py the file backing the samba-dc-container
command.
Move a bunch of that common functionality to common.py a new file
for these common things.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Avoid importing with noqa comments when importing a file just for the
commands it provides. Add a function to "include" (aka import for
commands) to the command builder type.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Use the new include method(s) and avoid top-level imports that just
exist to pull in decorated command functions.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Now that the main samba-container entrypoint is cleaned up,
allow the dc script entry point to be a bit more like the regular
commmands when set up.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Split the job of the formatting testenv that was previously running
both flake8 and black. Let formatting focus on code formatting only
(using black) and add a new testenv "flake8" for running said tool.
This avoids overloading the testenvs and allows for more flexibility
like running these envs with tox's --parallel option.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Copy link

mergify bot commented May 16, 2025

rebase

✅ Branch has been successfully rebased

@phlogistonjohn
Copy link
Collaborator Author

mergify seems to be to be ingoring Xavi's review because he didn't have write perms on this repo before today. I need one more review, or I can just force it by putting on the label... I may wait a little while... but not too long :-)

@mergify mergify bot merged commit cb441b5 into samba-in-kubernetes:master May 16, 2025
9 checks passed
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