-
Notifications
You must be signed in to change notification settings - Fork 42
Conversation
@@ -0,0 +1,30 @@ | |||
#!/usr/bin/env bash |
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.
Do you prefer a script (mainly used by the CI) or a Make goal in the e2e directory, so that developers can run them locally? There are already a few examples for fleet in that Makefile.
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.
Will the retry in the CI something that could be also included in the make goal? See https://github.com/elastic/e2e-testing/pull/705/files#diff-12ed1626622bd381afb059882db321d0014a9e9e394c4ca6ff4731887d912076R20-R28
No strong opinion whether to use a sh or a make goal as long as it's just a simple call that the consumer of this project needs to care about
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.
Mmm that's right: I did not look at the retry command. Well, if it's simple as it is, then I'm OK with it.
BTW, this script is needed because we need to install the dependencies, and it's done in the Jenkinsfile, right? Wdyt about simplifying the Jenkinsfile to execute the same as in the consumer, with one script? It will simplify the Jenkinsfile (less calls) and the future script will be self-contained, following same strategy as the script in this PR.
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.
BTW, this script is needed because we need to install the dependencies, and it's done in the Jenkinsfile, right?
Yes
Wdyt about simplifying the Jenkinsfile to execute the same as in the consumer, with one script?
I was tempted to do so, but it might require further surgery, let me give a go
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.
Let me know if we can collaborate there
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, I restructure the scripts so this is now a dummy one to call the functional-test.sh with some parameters, we could even potentially remove it, though I like the idea of using this contract
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
@@ -325,11 +325,6 @@ def generateFunctionalTestStep(Map args = [:]){ | |||
if(isInstalled(tool: 'docker', flag: '--version')) { | |||
dockerLogin(secret: "${DOCKER_ELASTIC_SECRET}", registry: "${DOCKER_REGISTRY}") | |||
} | |||
retry(3){ |
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.
.ci/scripts/functional-test.sh
is the one in charge to call this script
@@ -107,11 +107,6 @@ pipeline { | |||
steps { | |||
deleteDir() | |||
unstash 'source' | |||
retry(3){ |
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.
.ci/scripts/functional-test.sh
is the one in charge to call this script
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 believe this file can be removed as it's not used anymore: let's do it in another PR
## Install the required dependencies for the given SUITE | ||
.ci/scripts/install-test-dependencies.sh "${SUITE}" | ||
|
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 calls the script though the one in charge to retry is the install-test-dependencies.sh
itself
* Metricbeat goal (#705) * Update .ci/scripts/metricbeat-test.sh
* Metricbeat goal (#717 * Update .ci/scripts/metricbeat-test.sh
* Metricbeat goal (#705) * Update .ci/scripts/metricbeat-test.sh
What does this PR do?
Enable metricbeat script to delegate all the logic to this repo then the consumer only need to call the script
Why is it important?
Simplify the consumer calls
Checklist
[ ] My code follows the style guidelines of this project[ ] 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 run the Unit tests for the CLI, and they are passing locally[ ] I have noticed new Go dependencies (runmake notice
in the proper directory)Author's Checklist
How to test this PR locally
Run the script
.ci/scripts/metricbeat-test.sh
Related issues
Relates elastic/beats#23854