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.

Sign up for GitHub

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

Jump to bottom

fix(updatecli) do not reset cron pipeline triggers when no cron expression is passed #315

Merged
merged 3 commits into from
Mar 8, 2022
Merged

fix(updatecli) do not reset cron pipeline triggers when no cron expression is passed #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbbad4e
fix(updatecli) do not reset cron pipeline triggers when no cron expre…
dduportal Mar 6, 2022
cd96ec9
nitpicks
lemeurherveCB Mar 6, 2022
7e27f86
Merge branch 'master' into fix/crontriggers
dduportal Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions vars/terraform.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,16 @@ def call(userConfig = [:]) {
final String sharedToolsSubDir = '.shared-tools'
final String makeCliCmd = "make --directory=${sharedToolsSubDir}/terraform/"

// Only define a cron trigger on the "principal" branch
if (isBuildOnProductionBranch && finalConfig.cronTriggerExpression) {
properties([
pipelineTriggers([
cron(finalConfig.cronTriggerExpression)
]),
])
}

properties([
// Defines a cron trigger only on the production branch
pipelineTriggers(isBuildOnProductionBranch ? [cron(finalConfig.cronTriggerExpression)] : []),
// Only run 1 build at a time, on a given branch, to ensure that infrastructure changes are sequentials (easier to audit)
disableConcurrentBuilds(),
// Only keep build history for long on the principal branch
Expand Down
23 changes: 14 additions & 9 deletions vars/updatecli.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
def call(userConfig = [:]) {
def defaultConfig = [
action: 'diff',
config: './updatecli/updatecli.d',
values: './updatecli/values.yaml',
updatecliDockerImage: 'jenkinsciinfra/helmfile:2.3.0',
containerMemory: '512Mi'
action: 'diff', // Updatecli subcommand to execute
config: './updatecli/updatecli.d', // Config manifest used by updatecli (can be a file or a directory)
values: './updatecli/values.yaml', // Values file used by updatecli
updatecliDockerImage: 'jenkinsciinfra/helmfile:2.3.0', // Container image to use for running updatecli
containerMemory: '512Mi', // When using 'updatecliDockerImage', this is the memory limit+request of the container
cronTriggerExpression: '', // When specified, it enables cron trigger for the calling pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems really wrong. A library function should not be mutating the definition of the calling job. Why is there here and how can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some jobs need to execute the "updatecli" phases daily, some other weekly, while all jobs have a "main phase" (build/test/deploy-if-branch-main) that should run every 12h or 30 min.
I'm not sure how to implement this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A library function should not be mutating the definition of the calling job

I'm sorry but I don't understand what is wrong in this, and why is the pipeline library system allowing me to do this. Is there any writing or explanation that I could get to understand and avoid other user doing the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the same time, we have jenkins-infra/helpdesk#2778 incoming that should help on this topic: for the cost of "yet another pile of job-dsl to maintain", we will define the updatecli tasks on another pipeline, meaning different cron triggers on another job. That should simplify a lot the existing pipelines but at the cost of maintenance of jobs.

@jglick would this be a solution to "get out" of the mutating pattern that you describe as wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess. I do not understand the meaning of the current setup; it is certainly not using a commonplace idiom. In particular I do not recommend calling shared library functions from supposedly Declarative scripts. Technically it is supported, but when the function is doing something that overlaps with Declarative’s domain—and running the properties step certainly qualifies—you are running a weird and untested system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not recommend calling shared library functions from supposedly Declarative scripts

🤔 That triggers a lot of questions to me and I need help:

  • How should I share a "block of declarative instructions" between my pipelines? (e.g. I got a stage which is always the same on all my pipelines: how to reuse and update it)
  • If I learn groovy and start switching my declarative pipelines to scripted, would it be a bad thing to define properties[pipelineTriggers[cron('@daily')]] in this library, with properties[pipelineTriggers[cron('H/30 * * * *')]] defined in the Jenkinsfile (in scripted) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess. I do not understand the meaning of the current setup;

  • The project https://github.com/jenkins-infra/kubernetes-management/ (for instance) has it's main workflow being a matrix (each branch is deploying stuff on a different environment): I call it "main task".
  • "Main task" should run every 30 minutes.
  • "Main task" is defined in Jenkinsfile_k8s.
  • But this project also need to run updatecli tasks:
    • updatecli diff <whatever> on each build, to ensure that the code does not break it's "updatecli config" (manifests in ./updatecli/*yaml
    • updatecli apply <same whatever> once per day to open our PR for updating dependencies

So the pipeline in Jenkinsfile_k8s has a phase updatecli and then the matrix.
And since updatecli need to reuse its logic across all of the repositories of jenkins-infra/ , then it uses the shared library here for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

How should I share a "block of declarative instructions" between my pipelines?

Well, you can do that with a library call if you want to. But better to limit it to sh, junit, and the like. Certainly not properties.

would it be a bad thing to define properties[pipelineTriggers[cron('@daily')]] in this library

Generally seems unwise to run the properties step from a library. Remember that every call redefines the set of properties associated with the job. So if there are multiple such steps run in one build, you would need to ensure that the last one takes into account every property desired from previous ones.

Really not designed to be used this way. Normally should be run just once, near the start of the main pipeline definition. Why would you want to do this from a library to begin with? Does not make much sense to me. If you really want to have a library define a totally distinct variant of a job for certain branch patterns, then you can do this (in Scripted syntax!) if you are careful:

if (BRANCH_NAME == 'updates') {
  someLibraryCallDefiningWholePipeline();
  return;
}
// regular Pipeline follows…

Again I have no understanding of how you are integrating updatecli or why, so this is just generic advice.

Copy link
Member

Choose a reason for hiding this comment

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

In complement to Damien's comments, the updatecli part has been integrated that way because using multiple multibranch pipelines on the same repo wasn't something we knew how to/could implement. So mixing main work and maintenance tasks in the same pipeline was the solution chosen at that time.
Separating them is planned/in progress: jenkins-infra/helpdesk#2778

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks for the explanations and the time @jglick . We understand better the problem that you describe. We are going to keep this behaviour for a short time until we are able to move the updatecli logic on its own multibranch pipeline.

]

// Merging the 2 maps - https://blog.mrhaki.com/2010/04/groovy-goodness-adding-maps-to-map_21.html
Expand All @@ -16,10 +17,14 @@ def call(userConfig = [:]) {
// Do not add the flag "--values" if the provided value is "empty string"
updatecliCommand += finalConfig.values ? " --values ${finalConfig.values}" : ''

// Define a cron trigger only if it's requested by the user through attribute
properties([
pipelineTriggers(finalConfig.cronTriggerExpression ? [cron(finalConfig.cronTriggerExpression)] : [])
])
// Define a cron trigger only if requested by the user through attribute
if (finalConfig.cronTriggerExpression) {
properties([
pipelineTriggers([
cron(finalConfig.cronTriggerExpression)
])
])
}

// The podTemplate must define only a single container, named `jnlp`
// Ref - https://support.cloudbees.com/hc/en-us/articles/360054642231-Considerations-for-Kubernetes-Clients-Connections-when-using-Kubernetes-Plugin
Expand Down