-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
ci: improve linting speed #22103
ci: improve linting speed #22103
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
89ad5b9
to
be932d7
Compare
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 is great, my only concern is that we may be losing some check, or some condition to trigger some check. But the improvement may worth the risk of needing to solve or readjust some things in the following days/weeks.
After this change we could review the targets we have, there are many check/lint/fmt/update... targets and I am not sure if we need all of them.
developers will know if the PR pass the basics in 5 min(Linting)
That's a lot to say 🙂 We have to take into account that this 5 mins linting is only doing a very basic linting of the code. Generation of files (update and check targets) is the heavier part, the one that is easier to forget when committing, and will be checked later, taking some extra minutes.
I wonder if starting the stages in parallel may take much more resources. i.e a change in libbeat can trigger all builds, lint stage in libbeat might fail, but many other heavy stages for metricbeat, filebeat... are running. Could we abort and mark the build as failed immediately after any of the lint stages fail?
@@ -11,5 +11,7 @@ when: | |||
tags: true ## for all the tags | |||
platform: "linux && ubuntu-18" ## default label for all the stages | |||
stages: | |||
lint: | |||
make: "make -C deploy/kubernetes all" |
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 check should be executed when libbeat/docs/version.asciidoc
changes.
(Long-term we should probably stop commiting these files and move them to docs build)
And I guess we need to check for changes so it detects anything.
make: "make -C deploy/kubernetes all" | |
make: | | |
make -C deploy/kubernetes all; | |
make check-no-changes; |
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.
@kuisathaverat I see you have added the trigger on changes in the version file, thanks!
But the other point in this comment about adding a check is still pending to be addressed.
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.
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.
But the other point in this comment about adding a check is still pending to be addressed.
make -C deploy/kubernetes all
is not checking anything, I think we also need to run make check-no-changes
.
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.
yep, we need to runmake check-no-changes
also
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 you please add it?
cmd(label: 'make check', script: 'make check') | ||
cmd(label: "make check-python", script: "make check-python") | ||
cmd(label: "make check-go", script: "make check-go") | ||
cmd(label: "Check for changes", script: "make check-no-changes") |
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 will we decide what goes in this linting and what goes in the project-specific linting? For example this linting is not checking the license headers now.
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've moved the checks that are in a foreach in the main Makefile, so I think that every check that it is implemented on the beats should run only on beans. Indeed we should try to add check only to the beans to allow parallelize the task.
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.
Have you tried make -j <n>
?
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.
the VMs are not really powerful machines that do not have too many cores and are not dedicated hardware, so this could decrease a little the time(maybe 5-10min or less) of the task but far away from this approach that uses a VM for each stage (40min).
@@ -13,6 +13,11 @@ when: | |||
tags: true ## for all the tags | |||
platform: "linux && ubuntu-18" ## default label for all the stages | |||
stages: | |||
Lint: |
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.
Nit. This stage is upercased in some files, and lowercased in others.
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.
yep I saw it on the GitHub checks
x-pack/packetbeat/Jenkinsfile.yml
Outdated
@@ -13,6 +14,12 @@ when: | |||
tags: true ## for all the tags | |||
platform: "linux && ubuntu-18" ## default label for all the stages | |||
stages: | |||
# TODO confirm this works |
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.
Does it work? 🙂
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 deleted the file is not used, @v1v has to confirm it
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.
It is not used indeed, but I left there in case it will be supported in the future, so we can delete it for simplicity
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 to re-add the file because there is a magefile on the master branch, I've to check if we can backport this to all branches
/test |
💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # x-pack/packetbeat/Jenkinsfile.yml
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # Jenkinsfile.yml # auditbeat/Jenkinsfile.yml # filebeat/Jenkinsfile.yml # heartbeat/Jenkinsfile.yml # journalbeat/Jenkinsfile.yml # libbeat/Jenkinsfile.yml # packetbeat/Jenkinsfile.yml # x-pack/auditbeat/Jenkinsfile.yml # x-pack/elastic-agent/Jenkinsfile.yml # x-pack/filebeat/Jenkinsfile.yml # x-pack/functionbeat/Jenkinsfile.yml # x-pack/libbeat/Jenkinsfile.yml # x-pack/packetbeat/Jenkinsfile.yml
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # x-pack/packetbeat/Jenkinsfile.yml
Hey @kuisathaverat, there was a pending issue #22103 (comment), it seems to me that we are losing this check after this change. Could you please add it? |
* upstream/master: (93 commits) Update commands used in the quick start (elastic#22248) Add interval documentation to `monitor` metricset (elastic#22152) [CI] enable x-pack/packetbeat in the CI (elastic#22252) Fix awscloudwatch input documentation (elastic#22247) Add support for different Azure Cloud environments in the metricbeat azure module (elastic#21044) [CI] support windows-2008-r2 (elastic#19791) protect against accessing undefined variables in sysmon module (elastic#22236) [CI] archive only if failed steps (elastic#22220) Add pe fields to Sysmon module (elastic#22217) [CI][flaky] Support 7.x branches and PRs (elastic#22197) Perfmon - Fix regular expressions to comply to multiple parentheses in instance name and object (elastic#22146) ci: improve linting speed (elastic#22103) Move cloudfoundry tags with metadata to common metadata fields (elastic#22150) [Docs] Update custom beat docs (elastic#22194) [Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175) Update shared-autodiscover.asciidoc (elastic#21827) [DOCS] Warn about compression and Azure Event Hub for Kafka (elastic#21578) [CI][flaky] reporting for PRs in GitHub (elastic#21853) [Packetbeat] Create x-pack magefile (elastic#21979) [Elastic Agent] Fix deb/rpm installation (elastic#22153) ...
* upstream/master: (93 commits) Update commands used in the quick start (elastic#22248) Add interval documentation to `monitor` metricset (elastic#22152) [CI] enable x-pack/packetbeat in the CI (elastic#22252) Fix awscloudwatch input documentation (elastic#22247) Add support for different Azure Cloud environments in the metricbeat azure module (elastic#21044) [CI] support windows-2008-r2 (elastic#19791) protect against accessing undefined variables in sysmon module (elastic#22236) [CI] archive only if failed steps (elastic#22220) Add pe fields to Sysmon module (elastic#22217) [CI][flaky] Support 7.x branches and PRs (elastic#22197) Perfmon - Fix regular expressions to comply to multiple parentheses in instance name and object (elastic#22146) ci: improve linting speed (elastic#22103) Move cloudfoundry tags with metadata to common metadata fields (elastic#22150) [Docs] Update custom beat docs (elastic#22194) [Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175) Update shared-autodiscover.asciidoc (elastic#21827) [DOCS] Warn about compression and Azure Event Hub for Kafka (elastic#21578) [CI][flaky] reporting for PRs in GitHub (elastic#21853) [Packetbeat] Create x-pack magefile (elastic#21979) [Elastic Agent] Fix deb/rpm installation (elastic#22153) ...
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # Jenkinsfile # Makefile # auditbeat/Jenkinsfile.yml # filebeat/Jenkinsfile.yml # heartbeat/Jenkinsfile.yml # journalbeat/Jenkinsfile.yml # libbeat/Jenkinsfile.yml # metricbeat/Jenkinsfile.yml # packetbeat/Jenkinsfile.yml # winlogbeat/Jenkinsfile.yml # x-pack/auditbeat/Jenkinsfile.yml # x-pack/dockerlogbeat/Jenkinsfile.yml # x-pack/elastic-agent/Jenkinsfile.yml # x-pack/filebeat/Jenkinsfile.yml # x-pack/functionbeat/Jenkinsfile.yml # x-pack/libbeat/Jenkinsfile.yml # x-pack/packetbeat/Jenkinsfile.yml # x-pack/winlogbeat/Jenkinsfile.yml
* ci: improve linting speed (#22103) * ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # x-pack/packetbeat/Jenkinsfile.yml * Apply suggestions from code review Co-authored-by: cachedout <mike.place@elastic.co> Co-authored-by: cachedout <mike.place@elastic.co>
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # x-pack/packetbeat/Jenkinsfile.yml
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # Jenkinsfile.yml # auditbeat/Jenkinsfile.yml # filebeat/Jenkinsfile.yml # heartbeat/Jenkinsfile.yml # journalbeat/Jenkinsfile.yml # libbeat/Jenkinsfile.yml # packetbeat/Jenkinsfile.yml # x-pack/auditbeat/Jenkinsfile.yml # x-pack/elastic-agent/Jenkinsfile.yml # x-pack/filebeat/Jenkinsfile.yml # x-pack/functionbeat/Jenkinsfile.yml # x-pack/libbeat/Jenkinsfile.yml # x-pack/packetbeat/Jenkinsfile.yml
* ci: improve linting speed (#22103) * ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # Jenkinsfile # Makefile # auditbeat/Jenkinsfile.yml # filebeat/Jenkinsfile.yml # heartbeat/Jenkinsfile.yml # journalbeat/Jenkinsfile.yml # libbeat/Jenkinsfile.yml # metricbeat/Jenkinsfile.yml # packetbeat/Jenkinsfile.yml # winlogbeat/Jenkinsfile.yml # x-pack/auditbeat/Jenkinsfile.yml # x-pack/dockerlogbeat/Jenkinsfile.yml # x-pack/elastic-agent/Jenkinsfile.yml # x-pack/filebeat/Jenkinsfile.yml # x-pack/functionbeat/Jenkinsfile.yml # x-pack/libbeat/Jenkinsfile.yml # x-pack/packetbeat/Jenkinsfile.yml # x-pack/winlogbeat/Jenkinsfile.yml * Apply suggestions from code review * Update Makefile * Update Makefile * Update Jenkinsfile * Apply suggestions from code review
* ci: improve linting speed * fix: use multiple agents for the matrix * fix: add python-env * fix: set make folder parameter * feat: move k8s check and dev-tools to their own stages * fix: lint in a single stage is faster * fix: suggested changes * chore:change triggers * chore: update for packetbeat # Conflicts: # Jenkinsfile.yml # auditbeat/Jenkinsfile.yml # filebeat/Jenkinsfile.yml # heartbeat/Jenkinsfile.yml # journalbeat/Jenkinsfile.yml # libbeat/Jenkinsfile.yml # packetbeat/Jenkinsfile.yml # x-pack/auditbeat/Jenkinsfile.yml # x-pack/elastic-agent/Jenkinsfile.yml # x-pack/filebeat/Jenkinsfile.yml # x-pack/functionbeat/Jenkinsfile.yml # x-pack/libbeat/Jenkinsfile.yml # x-pack/packetbeat/Jenkinsfile.yml
What does this PR do?
It splits the
make check
target to allow parallelization.Why is it important?
The
make check
takes about 15 min, let's see how much we can speed up it. Because we moved part of the Liting to the beats pipelines we have reduced the total build time about 40 min (1h 50min before the change now ~1h), and the first response to lest than 4 min (it was 15-17 min before the change), so developers will know if the PR pass the basics in 5 min(Linting)Before
After
Checklist
[ ] My code follows the style guidelines of this project[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry inCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.