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

Add a mechanism to suppress stack trace. #176

Merged
merged 4 commits into from
Feb 7, 2020

Conversation

jeffret-b
Copy link
Contributor

@jeffret-b jeffret-b commented Dec 6, 2019

Add a mechanism to control whether HttpResponses.error() shows the error details, particularly the stack trace. By default it doesn't show the stack trace, but the interface can be enabled to show it.

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable approach.

@jeffret-b
Copy link
Contributor Author

@Wadeck @daniel-beck

As we're discussing doing for the stack suppression listener in Jenkins core.
@daniel-beck daniel-beck self-requested a review January 14, 2020 23:43
@jeffret-b jeffret-b changed the title Add an optional filter to suppress stack trace. Add an system property to suppress stack trace. Jan 21, 2020
@jeffret-b
Copy link
Contributor Author

Let's see if incrementals are working yet.

@jeffret-b jeffret-b closed this Jan 21, 2020
@jeffret-b jeffret-b reopened this Jan 21, 2020
@jeffret-b
Copy link
Contributor Author

Try kicking off incrementals again.

@jeffret-b jeffret-b closed this Jan 22, 2020
@jeffret-b jeffret-b reopened this Jan 22, 2020
@daniel-beck
Copy link
Member

I wonder whether it would be nicer to use a similar pattern as I used for the RequirePOST error page that lets the application render a view: https://github.com/stapler/stapler/blob/7d5bf955f3f0d174e760c522359ad8321c946f97/core/src/main/java/org/kohsuke/stapler/interceptor/RequirePOST.java#L40-L63

Not sure we're in the correct level here to do this, though. That way, even Stapler's error pages (and thereby the error pages served by the application through HttpResponses#error) would be nicer looking.

As this extends the API of Stapler, bringing this idea up now instead of considering this just a future enhancement. Since nobody else uses Stapler, it would be easy enough to defer though.

@jeffret-b jeffret-b changed the title Add an system property to suppress stack trace. Add a system property to suppress stack trace. Jan 28, 2020
@jeffret-b jeffret-b changed the title Add a system property to suppress stack trace. Add a mechanism to suppress stack trace. Jan 28, 2020
@jeffret-b
Copy link
Contributor Author

I adopted Daniel's suggestion of tying the implementation back to Jenkins again, this time using the oops page. I was concerned that we would have things in the proper space and stack but it seems to work fine.

Please review @jvz @daniel-beck @Wadeck

@jeffret-b
Copy link
Contributor Author

It looks like this is ready to go. @jglick or @daniel-beck, who can help get this merged in and released?

@daniel-beck
Copy link
Member

daniel-beck commented Feb 5, 2020

Looks like Stapler is (now?) released to repo.j.o, and nobody has permissions to do this, which basically means Oliver Gondza and the Artifactory admins (KK, Tyler, Wadeck, and me) can do it. Or we use some of the security process to deploy into a private repo and them copy it over. 😆

(I removed @jglick's previous universal upload permission a while back.)

@jglick jglick merged commit 81f51ff into jenkinsci:master Feb 7, 2020
@jglick
Copy link
Member

jglick commented Feb 7, 2020

IIUC this is waiting for jenkins-infra/repository-permissions-updater#1420

@daniel-beck
Copy link
Member

@jglick Looks like the parent POM results in deployment to OSSRH after all? If so, could you release Stapler?

@jglick
Copy link
Member

jglick commented Feb 10, 2020

https://github.com/stapler/stapler/releases/tag/stapler-parent-1.259

@jeffret-b
Copy link
Contributor Author

Thanks for releasing that, @jglick!

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

Successfully merging this pull request may close these issues.

None yet

5 participants