-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Adds support of sessionId for External-Job-Monitor #8825
Adds support of sessionId for External-Job-Monitor #8825
Conversation
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
According to https://issues.jenkins.io/browse/JENKINS-70684 this class is probably obsolete. Maybe the documentation needs to be updated in the plugin to no longer point to using things from core |
@mawinter69 I think SetExternalBuildResultCommand requires us to store the logs and send them via SSH to the Jenkins Host as mentioned in the doc. But Hudon Main wraps our command, keeps the log, and sends via HTTP to Jenkins, which gives quite a bit of flexibility while using the External-Job-Monitor plugin. So, keeping this class or at least this functionality is better. |
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 tested your PR locally, it looks fine from a security perspective!
/label ready-for-merge This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |
Fine but as mentioned in Jira please instead move all of this functionality into some utility in the plugin, so we can delete the class from core. It does not belong here and should not be touched. |
Indeed. BTW I think all the logic here to retrieve a crumb ought to be deleted anyway, as it should only be needed if you are using a password rather than an API token. |
I was using the External-Job-Monitor plugin and following this Doc, but I was getting 403 responses like this:
On looking into the code, found a bug:
GET request to fetch
crumb
before POST request forpostBuildResult
has an extra'
, due to which GET request was giving a non-2XX response, and crumb fields were not getting enriched.Furthermore, I was still getting 403 responses, and then I found this change. which mentioned:
So, to make things work, I added the support for
sessionId
returned in the GET call (to fetch crumb) to be sent in the POST call forpostBuildResult
.And then getting the expected results:
To summarize, this PR has the following two changes:
Testing done
Testing is done as mentioned above.
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist