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-72509] show error message in progressive logs on 4xx status codes #8907
Conversation
when accessing the log of a running build and the job gets deleted or someone removes read permissions (without discover) the log just appends the output of the call to logText/progressiveHtml which is the 404 message from Jenkins. The same happens when one accesses the log of an agent and the agent is deleted. This is due to only checking for 403 status and ignoring other status codes from the 400 range.
496a35a
to
1ab9182
Compare
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.
Thanks!
@@ -38,8 +38,8 @@ Behaviour.specify( | |||
}, 1000); | |||
return; | |||
} | |||
if (rsp.status === 403) { | |||
// likely an expired crumb | |||
if (rsp.status >= 400) { |
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.
should this reload on server errors? it might get in a reload loop?
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.
server errors are already catched in the previous if (line 35)
Is the diagnosis of short-lived logs a relevant use case we should continue to support, e.g., cloud agent connection logs? If so, a forced reload when it goes away makes for a worse experience. Not saying no to this, but this seems to have notable downsides compared to e.g. showing a message summarizing the error (rather than inlining the entire page, see e.g. how form validation currently does it) and offering to reload. |
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.
Thanks!
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
when accessing the log of a running build and the job gets deleted or someone removes read permissions (without discover) the log just appends the output of the call to logText/progressiveHtml which is the 404 message from Jenkins.
The same happens when one accesses the log of an agent and the agent is deleted.
This is due to only checking for 403 status and ignoring other status codes from the 4xx range.
With this change all 4xx http status code will lead to a reload and then showing a proper 404 page in mentioned cases
See JENKINS-72509.
Testing done
Agents:
-> the page shows an error message instead of adding the 404 to the logoutput
Jobs:
Create a job, that sleeps for a few minutes
start the job and look at the console output (dito for consolFull)
In a separate window delete the job
-> the page shows an error message instead of adding the 404 to the logoutput
Create a job, that sleeps for a few minutes
start the job
As a different user that has job read permissions open the job log (use another browser)
as admin user remove read permissions for that other
-> the page shows an error message instead of adding the 404 to the logoutput
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).