Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
migrate xpack-metricbeat #38081
migrate xpack-metricbeat #38081
Changes from 79 commits
2b5dd7b
cf44101
e432525
0783eee
2a1de26
7e55863
cf55e5b
def5673
85bfec8
5af9510
90bec63
bd038c2
77b29b2
5b41d3b
2751e38
88800c8
4805ccb
e242568
d41b68d
5319753
32229ce
a09803e
271c788
cd2282d
8eba9dc
1781fc0
c12fe5a
dd1e93e
d0f433b
751403b
39caa7b
2c4eb33
5696ea2
b6347ec
ba45c92
eaaa095
4612e00
e709ccf
79c8f95
ef0bd84
873c7d8
ae65e7f
f8a2a09
35d8957
bf8df0d
0d28c94
724e1c2
44fae9b
b488f25
2e531f8
e440521
20d2184
5fba521
f5a6545
5e71d65
2aa3993
7545802
929553f
9303a2a
700f8c1
d649de3
56d24bf
ad0492a
d532855
5823883
6b3e9de
bd864ec
6ff685b
9907579
48e1c08
fd09867
0db0696
6c59680
1ca09df
b1fdc2c
25547b4
c96b847
1057a94
84e8305
141c990
3cf8aa0
f432822
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
After thinking a bit more about
defineModuleFromTheChangeSet
and how other places could use it. I thinkMODULE
could be set in thepre-command
hook.Therefore my former suggestion for adding
defineModuleFromTheChangeSet
in.buildkite/scripts/cloud_tests.sh
could be deleted.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 tested it here: c96b847
And if we move that right now to the
pre-command
, it will require moving topre-command
all dependencies, answered hereThere 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.
Then, with this approach, the
MODULE
env variable won't be set in the other stages. Hence, it will take longer to run the tests.If that's something to be done in phase2 that's totally fine with me. But PRs will be slow to be tested. Unless the
defineModuleFromTheChangeSet "${BEATS_PROJECT_NAME}"
is also called in all those tests using thewithModule: 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.
IIUC, this particular function should use the beats project folder.
directory
in Jenkinsfile is set inbeats/Jenkinsfile
Line 1133 in 3f46222
and
beats/Jenkinsfile
Line 1105 in 3f46222
project
means.Then
directory
is used inbeats/Jenkinsfile
Line 676 in 3f46222
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 changed the function and its call: 141c990 and 3cf8aa0
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, there is a missing context what's the reason for running this terraform clean up function. A comment can help in the future.
In addition, should this particular script run only if the stage
cloud
ran for thex-pack/metricbeat
? Or will it work regardless of the stages in all the beats in case they call the cleanup function?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.
Both of these functions will be run only when it's needed, for example: https://github.com/sharbuz/beats/blob/25547b4ef8c75e9c356573b39e0bc25c0bf5e022/.buildkite/scripts/cloud_tests.sh#L7
It will run when the script finishes with EXIT code, which means when it finishes successfully/unsuccessfully
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 understand the trap function.
My main concern is regarding the cleanup function itself. If the pipeline finishes with
exit 0
, thencleanup
will run, but if it fails, will the failure be reported, too? I'm not sure if that's what we want. Post-build steps for tearing down resources should not fail the build but make it unstable or notify the cloud resources manager with a message, IMO.My team enabled a cloud-reaper mechanism that runs async to delete any leftovers in any of the cloud providers we use. That's how we can avoid having stalled resources and failing builds if the cleanup failed.
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 got what you mean, In this case I would suggest using the additional "cleanup" step with Slack notification or pipeline with email notification. @v1v WDYT?
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.
To illustrate a bit more, the current implementation in Jenkins does not fail when tearing down the resources with terraform/docker:
beats/Jenkinsfile
Lines 945 to 950 in 3f46222
It uses
returnStatus: true
that means do nothing, seeSee https://www.jenkins.io/doc/pipeline/steps/workflow-durable-task-step/#sh-shell-script
You can get the same:
either you can
|| true
or use your additional cleanup with soft_fail in Buildkite.Sounds good to me. But let me ask @dliappis about his preference.
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.
done: f432822