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

Change rss feed build name #2877

Closed
wants to merge 3 commits into from
Closed

Change rss feed build name #2877

wants to merge 3 commits into from

Conversation

ragaller
Copy link

@ragaller ragaller commented May 8, 2017

Description

With #2845 (released with Jenkins 2.58), if no display name is explicitly set the feed title contains only the build number, which is not very useful for non project specific feeds like the global /rssFailed feed.

This commit restores the old behaviour and honours the new behaviour of pull request 2845.

Proposed changelog entries:

  • Use build display names in RSS feed titles as introduced with Change rss feed build name #2845, but also honour the pre 2.58 behaviour, if no custom display name is set.

With #2845 (released with Jenkins
2.58), if no display name is explicitly set the feed title contains only
the build number, which is not very useful for non project specific feeds
like the global /rssFailed feed.

This commit restores the old behaviour and honours the new behaviour of
pull request 2845.
@daniel-beck
Copy link
Member

This makes no sense to me. Why not always show the project name, via getFullDisplayName()?

@ragaller
Copy link
Author

ragaller commented May 8, 2017

I just wasn't aware of the getFullDisplayName solution and simply tried not to break the #2845 change. And it's not clear to me how I can test @tequillaz usecase.
So that was a safe solution for me.

Changed implementation after receiving feedback to pull request.
@daniel-beck
Copy link
Member

In code review I misunderstood what #2845 does. Removal of project name would not have been accepted by me, not sure about @oleg-nenashev .

@oleg-nenashev
Copy link
Member

Removal of project name would not have been accepted by me, not sure about @oleg-nenashev .

I missed it as well.

@@ -2422,7 +2422,7 @@ public String getEntryID(Run e) {

private static class DefaultFeedAdapter implements FeedAdapter<Run> {
public String getEntryTitle(Run entry) {
return entry.getDisplayName()+" ("+entry.getBuildStatusSummary().message+")";
return entry.getFullDisplayName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing with #2845 RSS feed will be with project name but not build status. This is not enough.
We need to see project name (for rss with multiple projects, what #2845 broke), build name and build status like it was before.

Copy link
Author

Choose a reason for hiding this comment

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

So my first commit aec91c4 would be the right choice then?
Or is there a more appropriate/elegant solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

your first commit isnt right becouse if default build name changed we will lost project build name in rss feed

@@ -2422,7 +2422,10 @@ public String getEntryID(Run e) {

private static class DefaultFeedAdapter implements FeedAdapter<Run> {
public String getEntryTitle(Run entry) {
return entry.getDisplayName()+" ("+entry.getBuildStatusSummary().message+")";
if (entry.hasCustomDisplayName()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Project name will lost if custom build name will set

@ragaller
Copy link
Author

ragaller commented May 9, 2017

Sorry, really didn't realize that I dropped the build status. Thank's for your patience.

@oleg-nenashev
Copy link
Member

#2878 has been integrated both PRs were similar after the fixes in this one.

@oleg-nenashev
Copy link
Member

Thanks @ragaller !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants