-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add new API GET methods #69
base: master
Are you sure you want to change the base?
Conversation
ah it looks like the PR-merge build failed due to an issue on the agent? i'll close and reopen the PR to induce a rebuild |
Thanks a lot @blanked ! I will try to review this PR as soon as possible. |
@AbhyudayaSharma , Any update on this feature? |
@kalyantag Sorry, I got busy with other things and forgot about this PR. I will review it this weekend. |
Thanks @AbhyudayaSharma |
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.
Hi @blanked! Thanks a lot for the PR. Sorry for the late review; I completely forgot about it.
The PR looks good. I just have a few questions/suggestions. Please let me know what you think about them.
@Restricted(NoExternalUse.class) | ||
public void doGetAgentRole(@QueryParameter(required = true) String name) throws IOException { | ||
Jenkins.get().checkPermission(Jenkins.ADMINISTER); | ||
JSONObject responseJson = new JSONObject(); |
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.
I think we can pass a true
to the JSONObject constructor so that we return null instead of an empty object ({}
). Or maybe we can return a descriptive error message saying that role does not exist.
http://json-lib.sourceforge.net/apidocs/jdk15/net/sf/json/JSONObject.html#JSONObject()
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.
Hmm..I would prefer not to return an error message and instead return either an empty object or null as I find it more inline with other services. I can change it to return null
src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java
Outdated
Show resolved
Hide resolved
src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyAPI.java
Outdated
Show resolved
Hide resolved
JSONObject responseJson = new JSONObject(); | ||
AgentRole role = FolderAuthorizationStrategyAPI.getAgentRole(name); | ||
if (role != null) { | ||
responseJson.put("name", role.getName()); |
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.
Would it be possible to let json-lib handle converting the AbstractRole objects to JSONObjects using JSONObject#fromObject
?
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.
To use JSONObject#fromObject
, the role class must be of type string (json formatted), map, etc. The current class doesn't so it'll require a refactor for the class to extend either of these classes it to work. I'll keep it as is? Or we can
responseJson.put("sids", role.getSids()); | ||
responseJson.put("permissions", getPermissionsFromRole(role)); | ||
} | ||
Stapler.getCurrentResponse().setContentType("application/json;charset=UTF-8"); |
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.
I think it is possible to directly return a JSONObject
from a doFoo
function like done here:
Line 274 in 5fc71be
public JSONArray doGetAllFolders() { |
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.
Oh I wasn't aware of that. Is that due to the @GET
annotation from stapler? I can try refactoring it and testing if it works
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.
@GET
is to restrict the call to only GET requests: https://github.com/stapler/stapler/blob/master/core/src/main/java/org/kohsuke/stapler/verb/GET.java
I think returning a JSON response is implemented here: https://github.com/stapler/stapler/blob/0a23a9000de1dafb6a16fa883504697443cc05e3/core/src/main/java/org/kohsuke/stapler/json/JsonHttpResponse.java
src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java
Outdated
Show resolved
Hide resolved
|
||
// Verifying that the method returns all 3 roles | ||
String response = page.getWebResponse().getContentAsString(); | ||
JSONObject json = JSONObject.fromObject(response); |
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.
Is it possible to convert the JSONObject
back to a FolderRole
or AbstractRole
object using http://json-lib.sourceforge.net/apidocs/jdk15/net/sf/json/JSONObject.html#toBean(net.sf.json.JSONObject,%20java.lang.Class) ?
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.
Similar to my comment before, the roles need to extend map/bean to work. It'll require a refactor of the existing class
Co-authored-by: Abhyudaya Sharma <[email protected]>
@blanked Thanks a lot! Looks like the build is failing because of missing
|
Oh the |
Ok I've implemented the suggestions. Can you take another look and see if the PR is good to go? Thanks @AbhyudayaSharma |
I think the build can be restarted by pushing an empty commit (`git commit --allow-empty`) or closing it, waiting for 2 minutes and then reopening. I also believe that there was a hook added to rebuild from comments on pull requests in last year's GSoC. I will try to find whether it is available on ci.jenkins.io
…________________________________
From: Hui Jun <[email protected]>
Sent: Saturday, 6 March 2021, 21:17
To: jenkinsci/folder-auth-plugin
Cc: Abhyudaya Sharma; Mention
Subject: Re: [jenkinsci/folder-auth-plugin] Add new API GET methods (#69)
Ok is there any way of retriggering the tests? It seems like there were problems on the agent for the build. Closing and reopening doesn't seem to trigger a new build
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#69 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACXQV3YDJYIBITKSPEIJW4TTCJFB7ANCNFSM4S5SCAPA>.
|
Ok I finally got a build to run successfully. I believe I've addressed all of the comments/suggestion. Let me know if there are any other comments/suggestions? Thanks! |
Hi @oleg-nenashev ! If you have some time, could you please take a quick look at this PR? |
Hi @blanked. Sorry for the delay. What would you think about exporting the entire plugin configuration as JSON as done in https://github.com/jenkinsci/folder-auth-plugin/pull/58/files#diff-237f6cdc333b75a3ea591296a855f117edf78bb4ec1f9cbbc2391811e672304d? |
I thought it'll be good to have
GET
methods as well. This will allow admins to write automation (eg. reports for publishing user access info) on the plugin without having to resort to script console.