Conversation
mtojek
left a comment
mtojek
left a comment
There was a problem hiding this comment.
I think that I might be missing the justification behind this PR. Are we trying to fix/address a specific issue?
| * contains the integration or some common changes then it will be true, | ||
| * otherwise false. | ||
| */ | ||
| def isPackageEnabled(integrationName) { |
There was a problem hiding this comment.
I'm not sure if I fully understand the intention here. Which part of the helper is the "smart" one (as in the PR title)?
There was a problem hiding this comment.
smart was just a name, easy to change.
I tried to explain what I did and why in the PR description, but in a nutshell, if I understood correctly, the packages should be triggered based on the below conditions:
- when a daily build as requested in Fix: don't include 8.x packages in stack 7.x jobs #2334 . I used daily to be aligned with the reason described in the PR.
- On a PR only if there are changes in the specific package, as done in Run "check" and "test" only for affected packages in PRs #474
- For branches done in Run "check" and "test" only for affected packages in PRs #474 (since only main is available, then env.branch_name=='main' instead(so I'll amend this)
- Allow to change the PR behaviour with a GitHub command, that's handy as
2)won't allow to do so. - On a PR if changes in the some common shared files (.ci/Jenkinsfile, go.mod...), this is handy to test the pipeline itself and avoid
2)
There was a problem hiding this comment.
Re 5:
What about dependabot updates? We need to check all integrations then. Imagine that there is a bug in elastic-package, and ~3/100 integrations may catch it.
There was a problem hiding this comment.
What about dependabot updates?
Since it changes go.mod then it triggers all the integrations. So that's also covered
| return env.GITHUB_COMMENT?.contains('all') || env.COMMON_CHANGES == 'true' | ||
| } | ||
|
|
||
| def isDailyEnabled(integrationName) { |
There was a problem hiding this comment.
A similar question here, why do we need a separate function for Daily?
@mtojek, see the why, there is a regression and all the packages are executed for every build (regardless, daily, branches and PRs) |
|
Most like I'm missing something, where is the regression? AFAIK the intention is to:
|
Maybe I'm wrong with my assumption but #2874 was a change that triggered everything, is that the expected behaviour? |
Yes, this is aligned with current rules. As that change didn't fit the condition, so it fell back to the "test-all" approach. Definitely, it's something we can improve. |
|
As a consequence I probably wrongly assumed there was something incorrect generally speaking :/ So, we can easily close this now if not needed :) |
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
|
3 participants
What does this PR do?
Run "check" and "test":
/test allWhy
Re-enable #474 since #2334 added a
regression and therefore all the integrations are always triggered.