-
Notifications
You must be signed in to change notification settings - Fork 12
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-30133] Workflow compatibility #5
Conversation
super(build, defaultEncoding, result, new BuildHistory(build, CcmResultAction.class)); | ||
public CcmResult(final Run<?, ?> build, final String defaultEncoding, final ParserResult result) { | ||
super(build, new BuildHistory(build, CcmResultAction.class, false, false), result, defaultEncoding); | ||
serializeAnnotations(result.getAnnotations()); |
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.
This change is not related to workflow, but it was needed after upgrading to analysis-core upstream snapshot.
See this change which requires a explicit call to serializeAnnotations(result.getAnnotations())
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 for the heads up! 👍
@reviewbybees |
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
🐝 |
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
🐝 |
} | ||
|
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.
@amuniz Unrelated change? jajajaja
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.
😄
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.
Wut? haha, haven't looked at the analysis core, and to be honest the CCM migration to the analysis was a copy+paste from another plug-in.
Do you reckon it won't break existing jobs?
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.
It should not. I took care of backward compatibility, but if you see something weird, tell me!
NOTE: the initial comment of this thread was a kind of joke from my teammate, you can ignore it.
@reviewbybees done |
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
@kinow please, consider this PR for merge if you think everything is fine. Thanks. |
@kinow upstream Analysis Core was released some days ago. This PR is un-blocked now. Do you think it could be merged? Thanks! |
Just to let you know that I haven't forgotten about this PR. Just busy with another Jenkins related development cycle :-) Will try to review/merge it in the next days. Sorry for the delay. |
Great @kinow! Thanks for your work. |
@@ -67,14 +69,31 @@ | |||
<dependency> | |||
<groupId>org.jvnet.hudson.plugins</groupId> | |||
<artifactId>analysis-core</artifactId> | |||
<version>1.71</version> | |||
<type>hpi</type> | |||
<version>1.73-beta-SNAPSHOT</version><!-- TODO: change to 1.73 before release --> |
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 believe 1.73 has been released (see https://github.com/jenkinsci/analysis-core-plugin/tree/analysis-core-1.73).
Can I change it to 1.73 after merging?
ps: sorry, it took me so long to review, that actually 1.74 is out as well.
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.
Yes, you can safely change it to 1.73.
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 @amuniz ! Here's the test steps that I followed:
- Created a simple job, using the XML files in src/test/resources
- Tested that it was working correctly, and the graph was generated OK as well
- Checked out pr/5 branch locally
- Executed the job again
- Everything worked OK, except the graph, that was empty
- Updated to 1.73 the analysis-core. Same error.
- Updated to 1.74, and now it worked!
Merging your pull request, but will bump analysis-core to 1.74.
Thanks and sorry for the delay to review and merge it.
Cheers,
Bruno
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.
It seems that a bug in trend graphs were fixed in analysis-core 1.74. I think it's safe to upgrade to 1.74 since there are no big changes (only a bug fix).
Thanks Bruno!
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.
Didn't have time to check it, but thanks for confirming there was an issue in 1.73.
3.1 has been released about 45 minutes ago. Should be available through the update centre in the next hours.
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.
Great!
[JENKINS-30133] Workflow compatibility
Releasing 3.1 with this change |
Downstream of jenkinsci/analysis-core-plugin#31
JENKINS-30133
This changes allow to use
CcmPublisher
in a Workflow script, for example: