Skip to content

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
@mergify
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
@phlogistonjohn phlogistonjohn deleted the jjm-cli-nibbles branch June 9, 2025 14:08
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