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

Allow selected scripts to be removed from approvals #202

Closed
wants to merge 14 commits into from

Conversation

EricBartusch
Copy link

There are times where we want to remove a few approved scripts, and removing everything seems a bit excessive. Editing the scriptApproval.xml requires a restart of Jenkins which causes an interruption during working hours and feels a little hacky.

I changed the tag where the approvals are listed from a textarea to a table. With the table, users can select rows and hit the "clear selected" button to remove the highlighted rows. This change required a revamp of the updateApprovedSignatures javascript method. The body of the table must be recreated instead of the value of a textarea. Also, the acl/dangerous approvals will conditionally either have just a table tag if there are no approvals to go in the box. The description text above the tables will always appear.

Related issue:
https://issues.jenkins-ci.org/browse/JENKINS-22660

Kind of related issues though they were worked around:
https://issues.jenkins-ci.org/browse/JENKINS-31344
https://issues.jenkins-ci.org/browse/JENKINS-38352

@AndreasBS
Copy link

Thank you for making this. I would really like to use this functionality, but would prefer not running this plugin from a fork. Any chance of this PR being merged any time soon?

@EricBartusch
Copy link
Author

Hey @AndreasBS, I'm guessing that this PR won't be merged. From what I understand, Jenkins won't need the script security plugin in the future. If you go to the 24th slide here, there are some slides about the new Jenkins Pipeline Engine. Since my PR is a pretty big change to how the plugin currently works, I can imagine it's just not worth merging.

@AndreasBS
Copy link

AndreasBS commented Nov 20, 2018

Hi @EricBartusch, that's too bad. Thanks for the effort and the fast reply. At least the new pipeline engine looks promising, so I guess we'll just wait for that.

@bitwiseman
Copy link
Contributor

@EricBartusch That future may or may not be a reality.
Could you merge master into this PR? That will let folks get an incremental build from the CI system and take this for a spin.

@bitwiseman
Copy link
Contributor

bitwiseman commented May 25, 2019

@EricBartusch Thanks for the quick turn around.

Folks can pick this up from
https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/script-security/1.59-rc854.9d94da814218/ if they want to give it a try.

@darxriggs
Copy link
Contributor

Can we make this feature happen somehow?

@jglick
Copy link
Member

jglick commented Aug 28, 2020

Is this covered by #300?

@EricBartusch
Copy link
Author

Looks like it, and it looks a ton better than what I had here.

@jglick
Copy link
Member

jglick commented Jan 7, 2022

Closing, pending someone with time to properly review proposed UI improvements.

@jglick jglick closed this Jan 7, 2022
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.

5 participants