-
Notifications
You must be signed in to change notification settings - Fork 27
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 imgr and merge iocmanager into one bash file #101
Conversation
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.
Added some minor questions/comments that apply to both files. Should be good as-is though.
HUTCH=${HUTCH:-$(get_hutch_name)} | ||
|
||
# Run if possible | ||
if [[ "$HUTCH" == "unknown_hutch" ]]; then |
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.
Stuff like this makes me nervous about knock-on affects of upstream changes to get_hutch_name
's output. This also does not catch e.g. --hutch xvs
typos, but it probably doesn't need to.
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, a quick function to check valid hutch names would be nice. Good quick issue for intern :)
Created #106
if [ `whoami` == "xppopr" ]; then | ||
window_id=`xdotool search --sync --onlyvisible --name 'IocManager'` | ||
# Do some window cleanup for xpp | ||
if [ "$(whoami)" == "xppopr" ]; then |
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 gave me a laugh. Maybe eventually we should have some uniform way of doing this across the various scripts and hutches.
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.
Created #107
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.
LGTM
Description
Motivation and Context
The bash scripts to access imgr and iocmanager should exist in one place only and engineering_tools is a convenient place because it houses many other commonly used scripts and is already on most peoples's PATHs. This is the first step in moving the scripts from the iocmanager package to engineering_tools. After all hutches have adopted a new release of engineering_tools containing these scripts, I will remove the no-longer-useful bash scripts from the iocmanager package.
How Has This Been Tested?
Interactively.
Shellcheck passes with only SC1091 messages.
Where Has This Been Documented?
Commented each section in both scripts.