Skip to content
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

Combine statuses Failing, Failed, Error, Warning, FailedToGet and MulipleJobsRunning into one "Problem" #4132

Merged
merged 3 commits into from Mar 22, 2023

Conversation

gskrobisz
Copy link
Member

@gskrobisz gskrobisz commented Mar 19, 2023

Changes:

  1. Statuses Failing, Failed, Error, Warning, FailedToGet and MultipleJobsRunning are replaced by one dynamic status ProblemStateStatus.
  2. Removed SimpleStateStatus.Undefined.
  3. For each of "Problem" statuses tooltip and description have the same value (to remove unnecessary "UI message clutter" - each status had different tooltip and description, almost the same words but different arrangement)

Other changes:

  1. Parameter delegate in OverridingProcessStateDefinitionManager now has no default value. For clarity it should be provided explicitly.

To discuss / hint needed:

  1. Better name and location for ProblemStateStatus.
  2. Better location for MultipleJobsRunning - it was part of Flink&K8 custom statuses, now it is in SimpleStateStatus.ProblemStateStatus which seems to be wrong. Maybe it is better to move ProblemStateStatus outside of SimpleStateStatus.
  3. Remove unnecessary icons (keep error.svg instead of failed.svg? keep both "just in case"?)
  4. Provide descriptive "how to handle problem and what to do next" infos when status "Problem" occurs. Current descriptions/tooltips do not provide helpful information what to do with the problem, e.g. "Scenario has been canceled but still is running" or "Scenario deployed in version ... (by ...), should be running".
  5. Should healthcheck deployed-not-running indicate occurences of "Problem" scenarios?

Issues moved to another PR
6. StateStatus.isDuringDeploy can be replaced with pattern match with DuringDeployStateStatus, we do not need this at StateStatus level.
7. ProcessState.isDeployed - why during deploy is recognized as deployed?

@gskrobisz gskrobisz closed this Mar 19, 2023
@gskrobisz gskrobisz reopened this Mar 19, 2023
@gskrobisz gskrobisz changed the base branch from staging to improvement/filtering_by_status_part2 March 19, 2023 16:33
@gskrobisz gskrobisz changed the title Combine statuses Failed, Error and FailedToGet Combine statuses Failed, Error, Warning and FailedToGet Mar 19, 2023
@gskrobisz gskrobisz force-pushed the improvement/filtering_by_status_part3 branch from 9b1d940 to 79ea8ce Compare March 19, 2023 16:36
@gskrobisz gskrobisz force-pushed the improvement/filtering_by_status_part3 branch from 79ea8ce to 34763d0 Compare March 19, 2023 17:44
@gskrobisz gskrobisz changed the title Combine statuses Failed, Error, Warning and FailedToGet Combine statuses Failing, Failed, Error, Warning and FailedToGet Mar 19, 2023
@gskrobisz gskrobisz force-pushed the improvement/filtering_by_status_part3 branch 2 times, most recently from 0fb6850 to 3c29026 Compare March 21, 2023 00:33
@github-actions github-actions bot added the docs label Mar 21, 2023
@gskrobisz gskrobisz changed the base branch from improvement/filtering_by_status_part2 to staging March 21, 2023 00:33
@github-actions github-actions bot removed the docs label Mar 21, 2023
@gskrobisz gskrobisz changed the title Combine statuses Failing, Failed, Error, Warning and FailedToGet Combine statuses Failing, Failed, Error, Warning, FailedToGet and MulipleJobsRunning into one "Problem" Mar 21, 2023
@gskrobisz gskrobisz force-pushed the improvement/filtering_by_status_part3 branch from 99516dd to 6074c4c Compare March 21, 2023 12:09
@github-actions github-actions bot added the docs label Mar 21, 2023
@gskrobisz gskrobisz marked this pull request as ready for review March 21, 2023 12:10
Copy link
Member

@arkadius arkadius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@lciolecki lciolecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I left some mirror comments.

@gskrobisz gskrobisz merged commit f0223a2 into staging Mar 22, 2023
16 checks passed
@coutoPL coutoPL deleted the improvement/filtering_by_status_part3 branch August 16, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants