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

Extended scheduled job status messages #603

Merged
merged 12 commits into from
Apr 2, 2024
Merged

Conversation

satr
Copy link
Contributor

@satr satr commented Mar 11, 2024

No description provided.

@satr satr self-assigned this Mar 11, 2024
@satr satr requested a review from nilsgstrabo March 11, 2024 14:35
Copy link

github-actions bot commented Mar 14, 2024


BREAKING CHANGES:
=================
/applications/{appName}/environments/{envName}/jobcomponents/{jobComponentName}/jobs/{jobName}/copy:post -> 200 - Response - Body<ScheduledJobSummary>.status<string> - Added possible enumeration(s) - Active
/applications/{appName}/environments/{envName}/jobcomponents/{jobComponentName}/jobs/{jobName}:get -> 200 - Response - Body<ScheduledJobSummary>.status<string> - Added possible enumeration(s) - Active
/applications/{appName}/environments/{envName}/jobcomponents/{jobComponentName}/jobs:get -> 200 - Response - Body<array[ScheduledJobSummary]>.status<string> - Added possible enumeration(s) - Active
Spec Definitions.ReplicaSummary.endTime<string> - Added property
Spec Definitions.ReplicaSummary.exitCode<integer.int32> - Added property
Spec Definitions.ReplicaSummary.podIndex<integer.int64> - Added property
Spec Definitions.ReplicaSummary.reason<string> - Added property
Spec Definitions.ReplicaSummary.startTime<string> - Added property
compatibility test FAILED: 8 breaking changes detected

Copy link
Contributor

@nilsgstrabo nilsgstrabo left a comment

Choose a reason for hiding this comment

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

I'm not sure we should add the last warning event to the replica's statusMessage. See my comments

Comment on lines +175 to +180
eventList, err := kubequery.GetEventsForEnvironment(ctx, kubeClient, appName, deployment.Environment)
if err != nil {
return nil, err
}
lastEventWarnings := event.ConvertToEventWarnings(eventList)
replicaSummaryList = getReplicaSummaryList(componentPods, lastEventWarnings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how useful it is to include the last warning event. There can be multiple warning events, and only showing the last will potentially hide other important events. Perhaps a better approach is to have an endpoint for the component and/or replica that returns events for the particular object. We can then show it in web console as an event list, similar to the event list on the environment page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative can be to show events within the replica page, but it will not show the rootcause of some issues with replica after it was deleted

Comment on lines +479 to +480
if len(replicaSummary.StatusMessage) == 0 && (replicaSummary.Status.Status == Failing.String() || replicaSummary.Status.Status == Pending.String()) {
replicaSummary.StatusMessage = lastEventWarning
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure its a good idea to set last warning event in StatusMessage. We should instead discuss if we shall show events per component and/or replica, as described in my other comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as above - to keep the failure reason in the radix-batch status, when replicas and events do not longer exist

@satr satr merged commit 44b55c6 into master Apr 2, 2024
6 checks passed
@satr satr deleted the extend-scheduled-job-status branch April 2, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants