Extract the e2e retry block into a composite action - #126
Open
sscarduzio wants to merge 2 commits into
Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Three call sites ran the suite through the same retry wrapper: both matrix jobs in all-e2e-tests.yml and the hand-dispatched job in targeted-e2e-tests.yml. All three pinned the same action SHA, the same cap and the same wait, and two carried a near-identical copy of the reason for the cap. The next change to the cap would have had to edit three places, and the copies had already drifted once. .github/run-e2e-tests holds it now. The call sites differ only in inputs: the env and the version, the mode, the two optional image tags, how many attempts, and the Electron flags. The runner invocation is the PARAMS array the targeted workflow already used, so an input left empty drops its flag. A composite action cannot read secrets, so the activation key is an input, as the S3 credentials already are for upload-videos. The targeted workflow gains retry_on: any, which it did not set before. At max_attempts 1 the retry never fires, so this changes nothing today. It is stated rather than inherited by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sscarduzio
force-pushed
the
ci/extract-run-e2e-tests-action
branch
from
September 10, 2026 10:12
a010321 to
eb32480
Compare
This comment was marked as outdated.
This comment was marked as outdated.
coutoPL
requested changes
Sep 10, 2026
Five points from the review. `env` said "elk-ror" or "eck-ror". Those are the directory names the runner resolves to, not the values it takes: --env accepts `docker` or `eck-<version>` and rejects the rest. `ror_es` said "image tag", which it is not. It is one field of the tag, `<elk>-ror-<this>`, and it takes a ROR version or a pre-build run tag. `ror_kbn` now points at that instead of repeating it. `activation_key` explained why a composite action needs the value passed in. That is about the mechanism, not about the input. The timeout comment claimed 2 x 75. max_attempts is an input, so the worst case is max_attempts x 75. The retry comment named the same two transients twice over. One sentence each now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
coutoPL
approved these changes
Sep 10, 2026
coutoPL
left a comment
coutoPL
left a comment
Collaborator
There was a problem hiding this comment.
one comment left, but I'm giving you the approve. But please fix it.
LGTM
| required: false | ||
| default: '' | ||
| ror_kbn: | ||
| description: 'The same for the Kibana image.' |
Collaborator
There was a problem hiding this comment.
Oh, just rewrite it here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Follow-up to @coutoPL's review on #121, which is merged. Rebased onto
master, so this carries only the extraction.The duplication
Three call sites ran the suite through the same retry wrapper —
prod-e2e-testsanddev-e2e-testsinall-e2e-tests.yml, and the hand-dispatched job intargeted-e2e-tests.yml. All three pinned the samenick-fields/retrySHA, the sametimeout_minutes, the sameretry_wait_seconds, and two carried a near-identical copy of the reason for the cap.The copies had already drifted once: one was corrected while the other kept a claim the first had retracted. #121 had to edit the cap in three places, and so would the next change.
The action
.github/run-e2e-tests, next todocker-memory-monitor,upload-videosandcleanup-disk-space. The call sites differ only in things that map onto inputs:env,elkmodeproddevror_es/ror_kbnmax_attemptselectron_extra_launch_args--disable-gpu--disable-gpuThe runner invocation is the
PARAMSarray thattargeted-e2e-tests.ymlalready used, so an input left empty drops its flag and one command string covers all three.56 lines of workflow removed, 24 added, and one place to change the cap.
The two decisions you flagged
ROR_ENT_ACTIVATION_TOKENis anactivation_keyinput. That is whatupload-videosalready does with the S3 credentials.retry_on. The targeted workflow did not set it, and folding it in gives itretry_on: any. Atmax_attempts: 1the retry never fires, so nothing changes today. It is now stated in the action rather than inherited by accident, and the comment beside it says the retry covers transient infrastructure and not a failing suite.🤖 Generated with Claude Code
Summary by CodeRabbit