-
Notifications
You must be signed in to change notification settings - Fork 110
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 restart/flash Thunderloop button to RobotInfo widget #3368
base: master
Are you sure you want to change the base?
Add restart/flash Thunderloop button to RobotInfo widget #3368
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.
Fixed indentation in this playbook -- not related to PR
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 playbook appears to be unused
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.
looks great, this is super useful! one small thing, how do we see if the flashing / restart succeeded or failed for a robot? Or if the ssh connection failed, I know the button enables itself again once the process finishes either way, but would be nice to have some sort of visual / color confirmation
def restart_thunderloop_finished(ansible_result: AnsibleResult) -> None: | ||
self.thunderloop_button.setEnabled(True) | ||
self.ssh_connected = ( | ||
AnsibleResult.RUN_UNREACHABLE_HOSTS not in ansible_result |
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.
nit: out of curiosity, why do we use the in
operator here? would a normal equals thing not work?
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.
AnsibleResult
is a Flag
so multiple flags could be set, hence you need to use the in
operator
if not dialog_accepted: | ||
return | ||
|
||
def restart_thunderloop() -> AnsibleResult: |
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.
nit: method doesn't only deal with restarting thunderloop from what I can see, so maybe run_playbook_helper
or something idk
same for the finished
method
also could you attach screenshots of the dropdown menu when its opened? if possible idk |
you don't lol It's kinda difficult to get the service status bc you can't really return stuff from the ansible playbook. I'll try redirecting the stdout of the playbook executor to the log widget in thunderscope |
…into william/thunderloop_restart
I recently updated Ansible and it seems to have led to some merge conflicts whoops. |
…into william/thunderloop_restart # Conflicts: # src/software/embedded/ansible/run_ansible.py
binaries or execute platform-specific tasks. | ||
""" | ||
|
||
DEFAULT_SSH_PASSWORD = "thunderbots" |
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.
we don't want to leak our secrets on the repo
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.
Lmao, we have leaked this through commit history
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.
whoops
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.
nice work! left some comments
self.ssh_connected = False | ||
|
||
# Process for running Ansible playbooks (to restart/flash Thunderloop) | ||
self.ansible_process = QtCore.QProcess(self) |
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.
nice, did not know this existed.
this looks good, maybe just need to field test? |
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
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.
looks good pending merge conflicts (also pending any more field testing if more is needed)
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
for not stall. |
Description
Added a restart/flash Thunderloop button to the RobotInfo widget. The button runs an Ansible playbook via
./tbots.py
on a background thread (to trigger a Bazel build of Thunderloop).On the first run, a dialog prompts you to enter in the robot IP/hostname and password to SSH into the robot. The SSH details are saved for future runs if Ansible manages to successfully connect to the robot.
Testing Done
Resolved Issues
Resolves #3334
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.h
file) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom
. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO
(or similar) statements should either be completed or associated with a github issue