-
Notifications
You must be signed in to change notification settings - Fork 135
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
Draft version to support workflow #108
Draft version to support workflow #108
Conversation
stchar
commented
May 18, 2016
- Broken backward compatibility to send In-progress status in freestyle project
- For testing purpose removed some logic in calling TokenMacro.expandAll(...)
- Working fine in jenkins2.0 + pipleine
- Tests are not yet updated.
@headcrabmeat Any updates on this? I am quite interested in reviewing and testing this fix. At this moment it seems that it is not passing CI. |
CI is broken as some tests are not passed but |
any news about this pull request? |
@headcrabmeat I compiled and installed the version with your fix. I can confirm it's working with my freestyle jobs (I did not notice any issue...yet :)).
I just noticed the plugin does not appear in the pipeline syntax helper. Anyway thanks for your work! EDIT: After some more tests, I found an issue with your patch: there is a crash when trying to save the Jenkins global configuration due to the fact that the new field you added (buildState) does not appear in the json. Please find the full stack attached: |
@@ -604,6 +626,7 @@ public boolean configure( | |||
prependParentProjectKey = formData.getBoolean("prependParentProjectKey"); | |||
|
|||
disableInprogressNotification = formData.getBoolean("disableInprogressNotification"); | |||
buildState = formData.getString("buildState"); |
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.
As mentioned in #108, this causes a crash in Jenkins global configuration page. I'm not familiar with Jenkins plugin development, but I'm not sure you have to query the buildState here as this method seems to be used only to keep global configurations.
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.
Also I think this is a bad architectural design to pass the build state to the notifier constructor. That's not a part of configuration, but a part of runtime call
Implemented with #115 |