-
Notifications
You must be signed in to change notification settings - Fork 4
feat: FIR-42655 firebolt node sdk draining engine status not recognized #129
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
feat: FIR-42655 firebolt node sdk draining engine status not recognized #129
Conversation
|
ptiurin
left a comment
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.
Otherwise looks 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.
Generally let's try to avoid bundling unrelated changes, this makes it harder to make sense of them when viewing change 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.
Agree, this were just something I did some time ago and forgot to push
| UPGRADING = "ENGINE_STATUS_SUMMARY_UPGRADING", | ||
| REPAIRING = "ENGINE_STATUS_SUMMARY_REPAIRING", | ||
| STARTING_INITIALIZING = "ENGINE_STATUS_SUMMARY_STARTING_INITIALIZING", |
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.
Do you have a link for these? I don't see them defined anywhere apart from v1 code paths in firebolt-analytics.
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 the comment above mentions, these are here only for type match, they are not real statuses for V2



Added missing engine V2 statuses for node resource manager.
Also added missing geography type test