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

[JENKINS-57903]- Refactor PromotionConditionDescriptor extension Point to make it Pipeline Compatible. #137

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

dernDren161
Copy link
Contributor

@dernDren161 dernDren161 commented Jun 20, 2019

NOTE: This PR is being built on top of this: #128 PR:
This PR is just an uplift for the original one. Besides #128 these are the additional features:

  1. PromotionConditionDescriptor made Pipeline Compatible.
  2. Other extension points (most of the PromotionConditions) also refactored since they were affected by the methods inside PromotionConditionDescriptor class.

Things included in #128 :

  1. Made PromotionBadge compatible with pipelines.
  2. Refactored other important extension points and modules.
  3. Created a new interface named "PromotionRun".

See JENKINS-57903.

Your checklist for this pull request

  • Pull request title represents the changelog entry which will be used in Release Notes. JIRA issue is a part of the title (see existing PRs as examples)
  • Please describe what you did. Short summary for reviewers. If the change involves WebUI, add screenshots
  • Link to relevant Jenkins JIRA issues or pull requests
  • Test automation, if relevant. We expect autotests for all new features or complex bugfixes
  • Documentation if relevant (new features). We want to use Documentation-as-Code, just put docs to README or to new Doc files
  • Javadoc for new APIs. Any public/protected method is considered as API unless annotated by Restricted (docs)
    ScreenShot of change:
    Screenshot from 2019-06-20 14-33-39

CC

@oleg-nenashev @MadsNielsen @bicschneider @jonbrohauge

@dernDren161 dernDren161 changed the title Final [JENKINS-57903]- Refactor PipelineConditionDescriptor extension Point to make it Pipeline Compatible. Jun 20, 2019
@dernDren161 dernDren161 changed the title [JENKINS-57903]- Refactor PipelineConditionDescriptor extension Point to make it Pipeline Compatible. [JENKINS-57903]- Refactor PromotionConditionDescriptor extension Point to make it Pipeline Compatible. Jun 20, 2019
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.

This PR is definitely on the right path. But we need test automation and some Javadoc patches before it can be merged

import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Declared outside GroovyCondition as it depends on the optional 'script-security' dependency
*/
@Extension(optional = true)
public final class GroovyConditionDescriptor extends PromotionConditionDescriptor {
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason known for this just maybe because of the Job integrity it required multiple descriptors in pipelines(maybe) , just that the code compiled correctly this way would it break sth if final is removed?

return isApplicable((AbstractProject)item, TaskListener.NULL);
}
return true;
}
public boolean isApplicable(AbstractProject<?,?> item) {
Copy link
Member

Choose a reason for hiding this comment

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

The old APIs should be deprecated

}
return true;
}

public boolean isApplicable(AbstractProject<?,?> item) {
Copy link
Member

Choose a reason for hiding this comment

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

remove? Default implementation should be doing that

Copy link
Contributor Author

@dernDren161 dernDren161 Jun 21, 2019

Choose a reason for hiding this comment

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

@oleg-nenashev okay so changes in the return is expected having only one?

@@ -1,10 +1,11 @@
package hudson.plugins.promoted_builds;
package hudson.plugins.promoted_builds.pipeline;
Copy link
Member

Choose a reason for hiding this comment

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

Javadoc needs to be added according to the acceptance criteria

import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.model.User;
import hudson.model.*;
Copy link
Member

Choose a reason for hiding this comment

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

Please do not squash imports in the production code.
Your IDE can be reconfigured to disable it

public boolean isApplicable(@Nonnull Job<?,?> item, TaskListener listener){
if(item instanceof AbstractProject){
return isApplicable((AbstractProject)item, TaskListener.NULL);
}else{
Copy link
Member

Choose a reason for hiding this comment

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

minor nitpick: formatting. Leave blank spaces for if and else statements like everywhere else.

Choose a reason for hiding this comment

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

Your IDE could be configured to do it for 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.

The current code breaks APIs for AbstractProjects

}

public boolean isApplicable(AbstractProject<?, ?> item) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Why? it will make it unusable

@@ -0,0 +1,7 @@
Contributing
Copy link
Member

Choose a reason for hiding this comment

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

The history got messed up after merge by me. Should not impact the delivery

import java.io.IOException;
import java.io.PrintStream;

public class AddPromotionBadge extends Recorder implements SimpleBuildStep {
Copy link
Member

Choose a reason for hiding this comment

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

This code is still in the production scope, needs to be moved

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

Successfully merging this pull request may close these issues.

4 participants