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

[JENKINS-73813] Show a notification when scheduling a build fails #9787

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

mawinter69
Copy link
Contributor

When a build is triggered via the UI (either on the job page or via the buildbuttoncolumn in a view) and the crumb is no longer valid, the tooltip was shown that the build was scheduled, although nothing happened.
This change will show a notification when scheduling failed.

image

See JENKINS-73813.

Testing done

Manual testing

  • setup jenkins with users
  • create a job without parameters
  • go to dashboard
  • in a second window logout the user
  • click on schedule build -> notification is shown

Proposed changelog entries

  • Show a notification when scheduling a build fails

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

When a build is triggered via the UI (either on the job page or via the
buildbuttoncolumn in a view) and the crumb is no longer valid, there was
still the tooltip shown that the build was scheduled, whereas nothing
happened.
This change will show a notification when scheduling failed.
@mawinter69
Copy link
Contributor Author

I've chosen to use a notificationBar in case of an error as the hoverNotification doesn't allow to change color.
One idea might be to use a notification also for the success case.

@janfaracik
Copy link
Contributor

janfaracik commented Oct 1, 2024

I've chosen to use a notificationBar in case of an error as the hoverNotification doesn't allow to change color. One idea might be to use a notification also for the success case.

I think it'd make sense to standardise on one, +1 on the notification.

Co-authored-by: Jan Faracik <[email protected]>
Copy link
Contributor

@janfaracik janfaracik left a comment

Choose a reason for hiding this comment

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

LGTM - thanks :)

@@ -23,3 +23,4 @@
Task_scheduled={0} scheduled
Schedule_a_task=Schedule a {1} for {0}
Schedule_a_task_with_parameters=Schedule a {1} with parameters for {0}
Task_schedule_failed=Failed to schedule build for {0}.
Copy link
Contributor

@janfaracik janfaracik Oct 1, 2024

Choose a reason for hiding this comment

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

Suggested change
Task_schedule_failed=Failed to schedule build for {0}.
Task_schedule_failed=Failed to schedule build for {0}

For consistency with the 'Build scheduled' content. Any thoughts?

@janfaracik
Copy link
Contributor

Couple linting issues.

@daniel-beck
Copy link
Member

This does not address the issue with Build Now from context menus (e.g., breadcrumb bar). Is that deliberate or just an oversight?

@mawinter69
Copy link
Contributor Author

This does not address the issue with Build Now from context menus (e.g., breadcrumb bar). Is that deliberate or just an oversight?

ah yes, that I overlooked

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 12, 2024
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 13, 2024
@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise labels Oct 13, 2024
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 13, 2024
@timja timja merged commit 8458d66 into jenkinsci:master Oct 14, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants