-
Notifications
You must be signed in to change notification settings - Fork 6
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(app): surface message and status when pod is unschedulable #1088
Conversation
7b0608f
to
2965ec7
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.
Looks good, just one comment you can disregard if the method is needed
@classmethod | ||
def from_value(cls, val: str): | ||
if not val: | ||
return None | ||
if not val.lower() in cls.list(): | ||
raise ValueError( | ||
f"Value {val.lower()} cannot be found in list {cls.list()}" | ||
) | ||
return getattr(cls, val.capitalize()) | ||
|
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 needed? You can already do e.g. ServerStatusEnum("running")
(for value) or ServerStatusEnum["Running"]
(for key)
And I don't see it used in the code.
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 will merge this but open another PR to remove this. I guess I thought I needed it and then figured out I can do what you suggest. This is not used at all.
Depends on SwissDataScienceCenter/amalthea#165
closes #1017
For now I have decided to use the "failed" status for unschedulable sessions. We can add a dedicated or different status in the future if this is confusing to users. IMO however it is nicer to keep the number of potential session statuses small and simple.