-
Notifications
You must be signed in to change notification settings - Fork 96
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
Update bashrc and postgres-connection roles #610
Update bashrc and postgres-connection roles #610
Conversation
Updates: 1. This update will now allow us to add the bashrc content to more than 1 user. However, the default behaviour is retained (for backwards compatibility) i.e., by default it adds the contents to the galaxy user. 2. Added ENVs required for certain gxadmin commands 3. Added new change_to_jwd function that uses the new get_jwd bash script. This script and the function accepts only the job id. The new alias is chg2jwd and also the old method chg2wd is available just in case.
@@ -0,0 +1,61 @@ | |||
#!/bin/bash | |||
# Description: Get the job working directory using the job id and the object store configuration XML file |
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.
You have a python script for that, isn't it? How does this script relate to it?
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 have a Python script for cleaning up JWD's. I can also tweak that script and this functionality to it, and we can call a sub-command get_jwd
via the command line that would return the JWD path and another sub-command cleanup_jwd
that would clean up the JWDs if you would like.
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 think it would make sense to have this logic only in one place and not in two different scripts. Especially if you are planning to upstream your python script, we could reuse it easily.
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 have totally updated the script. Here is the gist. Please have a look and share your review.
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.
@sanjaysrikakulam this looks largely ok, just a few small things. I guess its ready for trying to upstream it.
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.
@bgruening Created PR: galaxyproject/galaxy#15618
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.
Shall I remove this bash script and replace it with the python script (in the above PR) until the upstream merge it and release it?
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.
Sounds good!
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 have updated this PR. Please review everything and let me know if any changes are required.
Remove the bash script template Replace with the new python script Add PGHOST ENV to the bashrc
Now we can use the variables to add the configuration to any system user and use any galaxy database credentials
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.
As I understand we have to invoke the bashrc role for every user. Would it be possible to use a list instead?
Otherwise it looks good to me.
@@ -0,0 +1,4 @@ | |||
--- | |||
user_name: "{{ galaxy_user.name }}" |
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 think its good to use more specific variable names, to avoid unintended overwriting of other variables and less confusion in the vars files. e.g.
user_name: "{{ galaxy_user.name }}" | |
bashrc_user_name: "{{ galaxy_user.name }}" |
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.
Ansible does not support looping over a block
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.
All tasks in a block
should be moved out to another tasks file and then should be included in the main.yml
where we can loop. I can implement this since you don't like the idea of invoking the role for every user.
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 like that more, because the variables can stay in a vars file and are not in the playbook. Thank 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.
Here is the update 3b6594f
I have also updated the example above
This change will allow to include role once instead of including/invoking it for ever user. The required variables can now be defined as a list and if the users exist the required tasks will be applied.
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.
Thank you @sanjaysrikakulam
Look at this PR for changes: usegalaxy-eu#610
alias glg='journalctl -fu galaxy-gunicorn@* | grep -v -e "/api/upload/hooks" -e "/history/current_history_json"' | ||
alias glh='journalctl -f -u galaxy-handler@*' | ||
alias glw='journalctl -f -u galaxy-workflow-scheduler@*' | ||
alias glc='journalctl -fu galaxy-celery@*' |
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.
celery is not on the head nodes anymore, or is this role also used on the celery host?
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.
No, the role is not used in the celery.yml playbook. I think it's legacy code. I will remove it and add a new commit.
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.
Updated: 229c0cb
Updates:
galaxy
user. Please make sure that the user (and the home directory) already exists.Example:
gxadmin
commandschange_to_jwd
function that uses the new/usr/local/bin/galaxy_jwd
python script. This script and the function accept only the job id. The new alias ischg2jwd
and also the old methodchg2wd
is available just in case.The script
/usr/local/bin/galaxy_jwd
needs the following ENVs and a~/.pgpass
file to workENVs:
~/.pgpass
format: <pg_host>:5432:*:<pg_user>:<pg_password>Additionally, the
hxr.postgres-connection
role has been updated to add the required PG configuration to the bashrc file of any user (as long as the user is already present and the home directory is created). The update is backwards compatible (default vars are set accordingly to make sure the role retains its default behaviour).