-
-
Notifications
You must be signed in to change notification settings - Fork 136
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
fix(updatecli) do not reset cron pipeline triggers when no cron expression is passed #315
Conversation
…ssion is passed Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
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 think ok
Let's wait for 2-3 advises before merging so we're sure to apply common good practises if it's ok for y'all |
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 |
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.
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?
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.
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?
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.
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?
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.
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?
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 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.
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 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, withproperties[pipelineTriggers[cron('H/30 * * * *')]]
defined in theJenkinsfile
(in scripted) ?
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 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.
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.
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.
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.
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
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.
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.
…ssion is passed (jenkins-infra#315) * fix(updatecli) do not reset cron pipeline triggers when no cron expression is passed Signed-off-by: Damien Duportal <damien.duportal@gmail.com> * nitpicks Co-authored-by: Hervé Le Meur <hlemeur@cloudbees.com>
If I understand correctly my mistake, this PR should fix jenkins-infra/helpdesk#2803 by only defining a cron trigger if there is a non empty expression string defined as argument.
It allows NOT resetting the list of cron triggers when there is already one defined at the pipeline level with no default value passed.