-
Notifications
You must be signed in to change notification settings - Fork 13
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
add a default template path for when we can't find a pipeline_template #195
base: master
Are you sure you want to change the base?
add a default template path for when we can't find a pipeline_template #195
Conversation
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.
Hey there! Thank you so much for your contribution! I didn't get a lot of time to look at this but went ahead and sent some questions/suggestions. I'd definitely like to hear your point of view.
Hope all is well!
@@ -29,5 +29,9 @@ THE SOFTWARE. | |||
<f:entry title="${%Config File Path}" field="scriptPath"> | |||
<f:textbox default=".yourconfig.yml"/> | |||
</f:entry> | |||
<f:entry title="${%Pipeline Template Path}" field="pipelinePath"> |
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 wonder if Default Pipeline Template Path
might be a better descriptor title? Perhaps renaming the field to defaultPipelinePath
if that makes sense?
@@ -0,0 +1,3 @@ | |||
<div> | |||
Relative default location of pipeline_template. |
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 might be good to describe the example. It doesn't have to be this suggestion but maybe something like this:
Relative default location of pipeline_template. | |
Relative default location of <code>pipeline_template</code>. This is useful when you have some repositories in an organization where you do not want to customize any values for the template or you cannot add a Config File to the repository. |
@@ -0,0 +1,3 @@ | |||
<div> | |||
Relative default location of pipeline_template. |
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.
Same suggestion as above 😄
Relative default location of pipeline_template. | |
Relative default location of <code>pipeline_template</code>. This is useful when you have some repositories in an organization where you do not want to customize any values for the template or you cannot add a Config File to the repository. |
String pipelineTemplateNotFound = | ||
String.format("Could not find a value for %s in %s", PIPELINE_TEMPLATE, scriptPath); | ||
throw new AbortException(pipelineTemplateNotFound); | ||
jenkinsfilePathString = pipelinePath; |
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 wonder if adding a log entry here might be useful and perhaps providing a different AbortException
at https://github.com/jenkinsci/config-driven-pipeline-plugin/pull/195/files#diff-cb4b4fb0cb84e1621bd3517151cb5d3308b9555208d5aed521263075d12603cbR195
What I'm thinking is that it might be hard to tell which of these code branches the plugin has taken and whether I have a misconfiguration in the plugin or if my repo simply isn't correct. Basically, seems like it'd be nice to help the user and admin understand.
Perhaps we could log whether the Config File was found, whether pipeline_template
was found, and/or if we're taking the Default Pipeline Template Path
@@ -21,15 +21,20 @@ | |||
public class ConfigDrivenWorkflowBranchProjectFactory extends AbstractWorkflowBranchProjectFactory { | |||
// TODO: Make this a parameter that users can adjust to their liking | |||
public static final String USER_DEFINITION_PATH = ".yourconfig.yml"; | |||
public static final String USER_DEFINITION_PIPELINE_PATH = "Jenkinsfile"; |
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.
Depending on whether we go with the name change for the field, it might be good to name this closer to what it is. 😄
public static final String USER_DEFINITION_PIPELINE_PATH = "Jenkinsfile"; | |
public static final String DEFAULT_PIPELINE_TEMPLATE_PATH = "Jenkinsfile"; |
This is something that would be very useful to me, as we are migrating off of the paid Cloudbees version of Jenkins and this is the behavior of their organization folder, where a jenkins file can be specified as part of the configuration instead of on a per-project basis. This allows us to use the same configuration file for CI, CD, and Testing but have seperate Jenkins files for each. |
@afalko could we get a review of this PR, and if not acceptable, let us know what else needs to be done to get it merged? |
Added a configuration option to configure the default path for
pipeline_template
instead of failing it when thepipeline_template
doesn't exist.Related to Issue: #25