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

add a default template path for when we can't find a pipeline_template #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AboorvaDevarajan
Copy link

@AboorvaDevarajan AboorvaDevarajan commented Jul 5, 2022

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Added a configuration option to configure the default path for pipeline_template instead of failing it when the pipeline_template doesn't exist.
Related to Issue: #25

@AboorvaDevarajan AboorvaDevarajan changed the title Config-driven-pipeline: Add a default template path for when we can't find a pipeline_template add a default template path for when we can't find a pipeline_template Jul 5, 2022
Copy link
Contributor

@justinharringa justinharringa left a 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">
Copy link
Contributor

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.
Copy link
Contributor

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:

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion as above 😄

Suggested change
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;
Copy link
Contributor

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";
Copy link
Contributor

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. 😄

Suggested change
public static final String USER_DEFINITION_PIPELINE_PATH = "Jenkinsfile";
public static final String DEFAULT_PIPELINE_TEMPLATE_PATH = "Jenkinsfile";

@benlamonica
Copy link

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.

@benlamonica
Copy link

@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?

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.

Add a default template path for when we can't find a pipeline_template
3 participants