-
Notifications
You must be signed in to change notification settings - Fork 26
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
ami2 script for lcls-1 hutches #212
base: master
Are you sure you want to change the base?
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.
I took a look because I was curious and it seems pretty clean to me with respect to code health.
The shellcheck CI has a bunch of minor suggestions, mostly based around quoting variables and some related things.
scripts/startami2
Outdated
get_status(){ | ||
grep ami2_ $STATUS_FILE | awk {'print $1"\t"$2"\t"$3'} | ||
} | ||
echo |
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.
Is this echo left here by accident, or does it do something?
Thanks for your comments Zach! Yes I noticed I did not have pre-commit installed on my checkout and was going to fix the syntax suggestions. |
One other tiny tiny thing- it'd be good to add this new script to the readme table |
Still need to figure out some issues with the mypy deamon for the second client to work smoothly... Let's hold on merging until I figure this out |
Ok, this is ready for review / merge! |
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 all seems reasonable to me!
I'm not in a position to evaluate the functionality but the code health looks good, I'm dropping a soft approval here.
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 the main thing missing from my point of view is including this script in the readme table
Description
Make a script to restart/stop ami2 and start more clients
Motivation and Context
Needed for smoother operation with ami2 in LCLS-I hutches
How Has This Been Tested?
With XPP DAQ and AMI
Where Has This Been Documented?
N/A