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

Review #127

Closed
wants to merge 1 commit into from
Closed

Review #127

wants to merge 1 commit into from

Conversation

dernDren161
Copy link
Contributor

@oleg-nenashev @jonbrohauge @bicschneider
Please look into the code.
The PromotionProcess is not quite fully functioning as i tried to make it an abstract class.
My questions:

I assume that the entire promotion process starts from PromotionCondition abstract class and the flow goes like:
PromotionCondition -> PromotionProcess -> Actual Conditions(Manual,Self...) -> PromotionProcess.....
After listing all the conditions in the DescribableList how do we actually match the input parameter for promotion to the exact class(eg: if user wants manual promotion, how does the plugin send the build/other information to the exact class(self,manual.java.....) for further processing? )
I found the getPromotionCondition method to be close. But this method is not called anywhere in the conditions.?
2)Also if the flow is incorrect please help rectify it. And after the respective promotion modules are called is:
public Future considerPromotion2
used for the final promotion??

Thank You!!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

As discussed at the previous KT session, such approach breaks binary compatibility. A more careful change is needed so that Java binary APIs stay intact.

@@ -31,7 +29,10 @@
* @deprecated
*/
@CheckForNull
public PromotionBadge isMet(AbstractBuild<?,?> build) {
public PromotionBadge isMet(Run<?,?> build, RunListener listener) {
Copy link
Member

Choose a reason for hiding this comment

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

Not binary compatible

@@ -47,9 +48,12 @@ public PromotionBadge isMet(AbstractBuild<?,?> build) {
* we know how a build was promoted.
* Null if otherwise, meaning it shouldn't be promoted.
*/
public PromotionBadge isMet(PromotionProcess promotionProcess, AbstractBuild<?,?> build) {
public PromotionBadge isMet(PromotionProcess promotionProcess, Run<?,?> build) {
Copy link
Member

Choose a reason for hiding this comment

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

not binary compatible

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 4, 2019

#128 provides a sample for a binary-compatible migration

@oleg-nenashev
Copy link
Member

Closing it to avoid confusion. We continue work in smaller pull requests

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

Successfully merging this pull request may close these issues.

2 participants