-
Notifications
You must be signed in to change notification settings - Fork 54
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
vdk-cicd: during a scheduled run publish_artifacts and release shouldn't run #1551
vdk-cicd: during a scheduled run publish_artifacts and release shouldn't run #1551
Conversation
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
This reverts commit 0dae32c.
Signed-off-by: murphp15 <murphp15@tcd.ie>
Please assign the #1254 to yourself. And linked it in teh ticket. |
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
Signed-off-by: murphp15 <murphp15@tcd.ie>
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.
could explain in the PR description what are the changes expected?
for example, I am wondering why the vdk-heartbeat publish/release stages are handled, and the vdk-control-cli are not?
Signed-off-by: murphp15 <murphp15@tcd.ie>
Fixed |
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.
Still don't understand why some gitlab ci yaml files are changed, and some are not.....for example https://github.com/vmware/versatile-data-kit/blob/main/projects/vdk-control-cli/.gitlab-ci.yml#L86 ?
for convenience, you can also link the gitlab schedule created, to save time (I went to find it on my own) |
the schedule is not actually created yet. |
It is changed, but I only changed it after you pointed it out. |
Is it possible to configure the rule to be "opt-in" - run on schedule only the jobs marked as such. With opt-in , it's easier to start with smaller set of jobs and add more incrementally until we are happy |
IMO opt out is better so we are testing more by default. I think the problem with current setup with both my solution and your suggested solution is that it touches soo many lines of code. However I only want to try that after I have identified all the edge cases etc.. Let me know what you think? |
Signed-off-by: murphp15 <murphp15@tcd.ie>
With opt-out if we add a new component/plugin build job, it would be automatically included in the scheduled nightly build. Which is a good thing. But if we add a new release job then it would be automatically included which is not what we want. But if we can extend a stage, as you suggest, then we can opt out "release" stage. Unless somebody releases in the wrong stages, there's no problem with wrong jobs being included. So I like this.. OK. Let's go with the proposed plan. First let's identify the what jobs need to be included and what not. |
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.
Looks good to me.
Since there's some risk. But I'd suggest that You do not merge it soon before clocking out and monitor the pipeline afterward. It's very unlikely that the new rules would break something but you never know
Signed-off-by: murphp15 <murphp15@tcd.ie>
Why
We want to be able to run scheduled jobs.
But as part of the scheduled jobs we want to skip
publish_artifact
jobs and alsorelease
jobs.This change should not have any impact on the pipeline and I will start running the scheduled jobs once we see the pipeline is still green for merge triggered builds.
In the future I might extract the rule into a shared abstract job. But that will be a later PR
What
Add a guard on all
publish_artifact
jobs and alsorelease
jobs so that they won't run if it is triggered by a scheduled operation.I also upgraded helm if we are testing everything we should try upgrade that too. The hope here is that it fixes #1252.
How has this been tested
Not much, mostly visual inspection.
closes #1254