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

Add click-compatible kerberos command #1499

Conversation

hankehly
Copy link

@hankehly hankehly commented Jun 16, 2022

Related: apache#22708

Summary

This PR adds click-compatible kerberos command.

Todo:

  • add click command
  • update unit tests

Screenshots (before/after)

convert-kerberos-command-to-click-cli

@hankehly hankehly marked this pull request as ready for review June 16, 2022 01:33
@hankehly
Copy link
Author

@blag Please review this PR at your earliest convenience.

Copy link

@blag blag left a comment

Choose a reason for hiding this comment

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

Looks really good, just one small, nice-to-have fixup.

@click.option("-k", "--keytab", metavar="KEYTAB", help="keytab", default=conf.get("kerberos", "keytab"))
def kerberos(principal, stdout, stderr, pid, daemon_, log_file, keytab):
"""Start a kerberos ticket renewer"""
print(settings.HEADER)
Copy link

Choose a reason for hiding this comment

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

Suggested change
print(settings.HEADER)
from rich.console import Console
console = Console()
console.print(settings.HEADER)

Ideally all output would flow through rich.console.Console.

Copy link
Author

Choose a reason for hiding this comment

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

Excellent, I'll remember that. Here's the before (bottom) and after (top)

Screen Shot 2022-06-17 at 8 52 26

@hankehly hankehly requested a review from blag June 16, 2022 23:54
@blag
Copy link

blag commented Jul 1, 2022

Thank you for all of the work you've done for this.

From this discussion, there is more groundwork and research necessary before we can evaluate rewriting the CLI with Click.

@blag blag closed this Jul 1, 2022
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.

2 participants