-
Notifications
You must be signed in to change notification settings - Fork 5
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
Auth #97
Auth #97
Conversation
Also managed to sneak in some code to replace all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Looks good! My only major comment is about the pollination command under auth. I think we should move it to the plugin itself. The core library should not have plugin specific commands IMO.
queenbee/cli/config/auth.py
Outdated
""" | ||
pass | ||
|
||
@add.command('pollination') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be added from queenbee-pollination not in pollination itself. Otherwise we will have to add several plugin commands to the core library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a command to add "pollination" style Auth to the queenbee config file. It will not serve as an entry point for other pollination commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is not the entry point command but don't you agree that this is a plugin specific command?
What stops you to move this command to queenbee-pollination? You can import add
and then add your new command when queenbee-pollination is loaded. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it does kinda feel like a plugin command. The difficulty I'm having is not with integrating the click/cli side as a plugin but managing passing the config
object which contains an Pollination api token and can create a JWT token that is then used to authenticate certain requests within recipes.
Essentially if I make this command a plugin then it feels I have to make this class which then makes it difficult (I think) to handle authentication to pollination servers when managing recipe dependencies. Part of the reason this is difficult is because we don't (currently) implement standard authentication mechanisms which we can expect to be reusable by other non-pollination servers.
I'm in agreement about moving pollination stuff into plugins in general. In this case however I'm not 100% sure of what the best way to integrate special Auth requirements into non-plugin commands. In this case it felt acceptable much like the way Docker provides default auth methods for Dockerhub. |
@mostaphaRoudsari I am going to merge this after having fixed the styling issues. I am in agreement with you about plugin based loading of pollination specific commands and modules/functions however it will take me time to think about and implement a decent plugin system that I won't yell at and rip apart in 2 weeks time. I have raised #98 to track this issue and we can plan some work to remove pollination specific modules and commands (it's only 1 class and one command for now) from queenbee. |
🎉 This PR is included in version 1.14.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This PR addresses generating packaged tar files without using a filesystem (for server side packaging) and optional authentication (from library and CLI).
fixes #93
fixes #95
fixes #96