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

Improved vault-show error message #101

Closed
wants to merge 2 commits into from
Closed

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Sep 21, 2016

Added more information to the NotFound error that may occur during
execution of vault-show. It was not clear whether the vault
really does not exist or it does not exist in a certain container.

https://fedorahosted.org/freeipa/ticket/5950

@MartinBasti MartinBasti self-assigned this Sep 21, 2016
@MartinBasti
Copy link
Contributor

NACK: you fixed only vault-show not other vault-* commands

NACK: I don't like the override of execute method (it should work for all vault-* commands automatically)

Is possible to override method handle_not_found of vault object? IMO which vault type is used can be determined by DN suffix (maybe it deserves a new method vault_type_from_DN()).

NACK: This is pure evil, pls keep better readability (use if-elif-else instead)

            if options.get('service'):
                container_type = 'service'
            else:
                container_type = 'shared' if options.get('shared') else 'user'

@MartinBasti
Copy link
Contributor

Oh realized that is not possible to create DN inside handle_not_found, because it does not take **kwargs

Probably we can extend handle_not_found with kwargs, but this decision needs broader audience.

Adding kwargs allows invocation options to be passed to
handle_not_found() to improve 'Not found' messages.

https://fedorahosted.org/freeipa/ticket/5950
@stlaz
Copy link
Contributor Author

stlaz commented Nov 25, 2016

Seems like nobody objected so far.

@@ -755,6 +755,28 @@ def get_container_attribute(self, entry, options):
elif entry.dn.endswith(DN(('cn', 'users'), container_dn)):
entry['username'] = entry.dn[1]['cn']

def handle_not_found(self, *keys, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe *keys and **options are better names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose you're right, fixed it here.

Added more information to the NotFound error that may occur during
execution of vault commands. It was not clear whether the vault
really does not exist or it does not exist in a certain container.

https://fedorahosted.org/freeipa/ticket/5950
@MartinBasti
Copy link
Contributor

I had discussion with @jcholast and he disagrees. This weird handling of DN should stay isolated in vault code and shouldn't be spreaded across the framework. I'm starting to think that we should close ticket as won't/can't fix instead of doing that bad code even worse.

@stlaz
Copy link
Contributor Author

stlaz commented Nov 28, 2016

WONTFIX then. There's no winning here.

@MartinBasti MartinBasti closed this Dec 8, 2016
@MartinBasti MartinBasti added the rejected Pull Request has been rejected label Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected Pull Request has been rejected
Projects
None yet
2 participants