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-18884] Remove 'People' view #9060
Conversation
<j:when test="${it.class.name=='hudson.model.AllView'}"> | ||
<l:task href="${rootURL}/asynchPeople/" icon="icon-user icon-md" title="${%People}"/> | ||
</j:when> |
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.
FTR this is a bug IMO, as AllView can exist in folders. Note how https://weekly.ci.jenkins.io/job/folder/ links to /asynchPeople/
.
@@ -31,7 +31,6 @@ THE SOFTWARE. | |||
<l:header /> | |||
<l:side-panel> | |||
<l:tasks> | |||
<l:task contextMenu="false" href="${rootURL}/asynchPeople/" icon="symbol-people" title="${%People}"/> |
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.
This is an awkward case as the people view basically represents the list of users, of which an individual user is an entry (which is why this is the first link in the sidepanel). Unless using the built-in Jenkins' own user database, admins no longer have a users list with this change. We should probably add a ManagementLink
before long to iterate over (actual) users.
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.
Unless using the built-in Jenkins' own user database, admins no longer have a users list with this change.
They do not have such a list currently. AsynchPeople
lists SCM users which are not in the security realm; and does not list users which are in the security realm but have no user record on disk. It is simply useless for this purpose.
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.
but have no user record on disk
Right, but anyone who's logged in once has such a record, which makes it a viable (if crappy and slow) entrypoint to getting to someone that's not directly entering the URL.
@@ -31,7 +31,6 @@ THE SOFTWARE. | |||
<l:header /> | |||
<l:side-panel> | |||
<l:tasks> | |||
<l:task contextMenu="false" href="${rootURL}/asynchPeople/" icon="symbol-people" title="${%People}"/> |
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.
Unless using the built-in Jenkins' own user database, admins no longer have a users list with this change.
They do not have such a list currently. AsynchPeople
lists SCM users which are not in the security realm; and does not list users which are in the security realm but have no user record on disk. It is simply useless for this purpose.
test/src/test/java/jenkins/security/stapler/GetterMethodFilterTest.java
Outdated
Show resolved
Hide resolved
It's not entirely clear what this test signifies, given the existence of `testTopLevelItemIsLegal`, but whatever.
@@ -0,0 +1 @@ | |||
blurb = The "People" view has been moved into the <a href="https://plugins.jenkins.io/people-view" target="_blank" rel="noopener noreferrer">People View</a> plugin. |
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.
blurb = The "People" view has been moved into the <a href="https://plugins.jenkins.io/people-view" target="_blank" rel="noopener noreferrer">People View</a> plugin. | |
blurb = The "People" view has been moved into the <a href="https://plugins.jenkins.io/people-view/" target="_blank" rel="noopener noreferrer">People View</a> plugin. |
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. /label ready-for-merge |
See JENKINS-18884 and #9052 (comment) for previous context. This is the core change corresponding to the creation of a new plugin as suggested in the linked comment.
Plugin sketch: https://github.com/daniel-beck/people-view-plugin
Usage check
A quick GH search indicates just two affected plugins:
usage-in-plugins only finds
jabber-server-plugin
.As
-M
and-C
:Result
jabber-server-plugin:1.9
Classes
hudson/model/View$People
Methods
jenkins/model/Jenkins#getPeople()Lhudson/model/View$People;
Testing done
Navigated to
/people/
and/asynchPeople/
, saw the new placeholder page.Ran https://github.com/daniel-beck/people-view-plugin/ as of daniel-beck/people-view-plugin@3bd4f1d and confirmed I'm seeing the plugin-contributed views.
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).