-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix typo: the past tense of shutdown is shutdown, not shutdowned #3659
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
Conversation
7625f5b to
3262fbc
Compare
| Error(false, "VM is in error"), | ||
| Unknown(false, "VM state is unknown."), | ||
| Shutdowned(false, "VM is shutdowned from inside"); | ||
| Shutdown(false, "VM is shutdown from inside"); |
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.
@GabrielBrascher Add database upgrade path to update any VMs in the old state to the new state name. (just a rare edge case).
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.
@rhtyd added a database upgrade schema from 4.13.0.0 to 4.13.1.0 upgrading VMs from state Shutdowned to Shutdown.
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.
@rhtyd does it look like a 4.13.1 fix? I had mapped it to 4.13.1.0, but I can target it to master/4.14.0.0; not really a bug fix, more likely an enhancement.
df067a3 to
9e2fa49
Compare
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.
Code LGTM
|
@blueorangutan package |
|
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-397 |
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 do approve of this change with one caveat:
If the state is used in automation by parsing logs or alerts this automation will break. I would like to see a small upgrade guide entry for that.
|
@DaanHoogland Makes sense adding it to the upgrade guide. |
|
@rhtyd are you ok with this PR? Database upgrade has been covered. |
|
Hi @GabrielBrascher let me review this and get back to you soon |
|
This could be considered major change @GabrielBrascher - can you move this to 4.13->4.14 upgrade path on master? |
|
No problem @rhtyd, I will move it to 4.14 |
9e2fa49 to
5db61c0
Compare
5db61c0 to
72b42ae
Compare
|
@rhtyd code has been updated, ported to master and added SQL UPDATE into |
|
Fixed conflict on |
|
@andrijapanicsb this one looks like a simple "fix" and has 2 LGTMs aiming it for 4.14. Can you please take a look? @rhtyd thanks for the review, are you ok with the code the way that it is after updating the upgrade path? |
|
Looks OK @GabrielBrascher and the schema update path as well. |
|
@blueorangutan package |
|
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-525 |
|
@blueorangutan test |
|
@andrijapanicsb a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@blueorangutan test |
|
@andrijapanicsb a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-693)
|
|
Let there be grammar! |
Description
Fixes: #3655
Types of changes