-
Notifications
You must be signed in to change notification settings - Fork 4
Decouple execution of roles #51
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
base: staging
Are you sure you want to change the base?
Conversation
Instead of always executing the tasks from both roles, deploy_hub and deploy_jhub, it would be nice to decide wich one each times. For example, when starting the cluster, we may not need to run the tasks for the deployment of the JupyterHub instance. Also, when setting up the JupyterHub instance (e.g. creating usernames), we may not want to rerun the steps to setup the cluster. The helm command may take too long. One way to allow the execution of only one of the roles, if needed, is using tags. With the new content in the playbook, we can now call it like this: $ ansible-playbook deploy_jhub.yml $ ansible-playbook deploy_jhub.yml --tags deploy_hub $ ansible-playbook deploy_jhub.yml --tags deploy_jhub
| - To run only the deploy_hub role: `ansible-playbook deploy_jhub.yml --tags deploy_hub` | ||
| - To run only the deploy_jhub role: `ansible-playbook deploy_jhub.yml --tags deploy_jhub` |
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.
Could we add a sentence for each role about what they do (as they have very similar names)
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.
Maybe something like this?
- To only run the role that creates the kubernetes cluster: ...
- To only run the role that set the JupyterHub stuff: ...
But with proper sentences?
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.
Yes, however both roles however are separate from creating the kubernetes cluster (see #39 (comment)). I think we need to review the roles and see whether we need to rename the roles to make them clearer on what they do, or combine them into one role.
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.
Do we want to merge them?
The whole point of decoupling their execution was to prevent a long wait (helm tasks?) when we only want to add a new username or something trivial like that.
Now, you have more experience than me. If that is never the case, and potential delays are minimal and acceptable, then this PR is not relevant, irrespective we merge the roles or not.
Up to you.
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.
@meoflynn @jose-caballero is this PR still active?
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.
Maybe. We are waiting to see what happens with the entire service. Depending on that, this may still be needed or can be deleted. So let's keep it for now as it is.
No description provided.