-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Snyk scanning pipeline fixes. #18499
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
Conversation
…ration is in other org and snyk CLI needs to be located in the repo.
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
| curl -sL --retry-max-time 60 --retry 3 --retry-delay 5 https://static.snyk.io/cli/latest/snyk-linux -o snyk | ||
| chmod +x ./snyk | ||
| export JAVA_HOME="/opt/buildkite-agent/.java/adoptiumjdk_21" | ||
| export PATH="/opt/buildkite-agent/.java/bin:$JAVA_HOME:$PATH" |
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.
Can we just source https://github.com/elastic/logstash/blob/main/.buildkite/scripts/common/vm-agent-multi-jdk.sh like we do elsewhere for prepping java env?
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.
Yup, thank you for pointing it, replaced with vm-agent-multi-jdk.sh
…nds to target only gradle belong to the plugin and ignores multi-projects
…oth all-project and project-name
donoghuc
left a comment
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.
Instead of using special branches and logstash_branch values for elastic_integration, I think we should be more explicit. If we find that is hard to maintain or keep updated in the future we can talk about other optimizations/workarounds. I think we are optimizing too early here and its starting to make the step generation code hard to reason about.
| if ! git clone --depth 1 --branch {logstash_branch} https://github.com/elastic/logstash.git; then | ||
| echo "Branch {logstash_branch} not found in logstash, skipping..." | ||
| rm -rf {work_dir} | ||
| exit 0 |
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.
Should this exit non zero?
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.
Yup, thanks for catching it. Removed if ... then if git clone workflow also fails (tested in the past)
| branches: [main, 'use-release-branches'] | ||
| ignore_branches: [7.x] | ||
| ignore_branches: [7.x] | ||
| logstash_branch: 'match-with-plugin-branches' |
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'm a bit hesitant to introduce this "magic" match-with-plugin-branches name. Fundamentally the only plugin that strives to sync up branch name with logstash is filter-elastic_integration. Clearly there is a case for mapping target plugin branch with logstash branch. Instead I think we should make this explicit. Specifically I think logstash_branch should be a map in which the keys are the branches in branches section and the values are the branch in logstash. We should add validation that when logstash_branch is present that branches is also present.
- logstash-input-dead_letter_queue:
branches: [main]
logstash_branch:
main: main
There is the same sentiment for the "magic" use-release-branches name. I really dont think that having these special cases throughout the code is worth the overhead. I think that maintaining static lists is less cognitive burden and makes us be more intentional about exactly what we are scanning.
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.
Yup, let's think about optimization. I have removed the magic use-release-branches entry 😁 and specified with plugin vs core branch pair.
… Use a precise {key, val} matrix to scan elastic_integration plugin with Logstash branch pair.
| - logstash-filter-geoip | ||
| - logstash-filter-jdbc_static | ||
| - logstash-filter-date: | ||
| requires_logstash_core: true |
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.
is requires_logstash_core a shorthand for a scan_branches map with a single key/value?
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 its a shorthand for:
- logstash-filter-date:
scan_branches:
- branch: main
logstash: main
You effectively get the "default" branch for both the plugin and logstash (where default branch is the configured default in the git repository).
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.
Ah, sorry I had to add such details in the PR description to explain. I will add.
Some plugins in the matrix require Logstash core gems, some do not. For example in this date-filter, its gradle requires logstashCoreGemPath, eventually rake task creates it.
In the past, I somehow used logstash_branch which doesn't make sense to me as we have introduced scan_branches map. So, replaced with requires_logstash_core to make it more understandable.
Basically, overall key points are:
- some plugins require LS core gems, we need to clone LS repo and define LS core path;
- some plugins have multiple active branches (e.g:
main,11.x) to scan; elastic_integrationplugin requires 1:1 branch map
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 have updated the PR description as well!
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.
That sounds like the same use case to me? the plugins (like date-filter) require the logstash repo to be cloned on the test runner. Presumably if there was a matrix of plugin version to logstash version we would want to do the same thing (make sure that logstash is available at a checked out version). I'm wondering why there are two different paths to effectively do the same thing.
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.
Yup yup, depth-first search became breadth-first search :)
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.
Thanks for asking clarifications.
donoghuc
left a comment
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.
Love this simplification f8da811
Thanks for iterating on the feedback. This is looking good to me! 🚀
💚 Build Succeeded
History
cc @mashhurs |
With this PR, the improvements are:
elastic_integrationis in theelasticorg/bin/bash: line 14: ./snyk: No such file or directorylogstash_branch: 'main') if requires (e.g:elastic_integrationor date-filter require logstash-core gems)With the matrix definition, the key points are:
requires_logstash_core: some plugins require LS core gems, we need to clone LS repo and define LS core path;logstashCoreGemPath, eventually:vendor => "gradle.properties"rake task creates it.branches: some plugins have multiple active branches (e.g: main, 11.x) to scan;scan_branches:elastic_integrationplugin requires 1:1 branch map