Skip to content
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

Trigger windows e2e tests when windows-related files change #36857

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

ArthurSens
Copy link
Member

Description

We've noticed that many of the CI failures on main come from Windows e2e tests. We want to be more proactive in catching those failures, so this PR changes the workflow triggering Windows e2e tests to ensure they run on Pull Requests that change Windows-related files, even if the label Run Windows is absent.

It does so by looking at changed files. If the word windows is present in the file name, it triggers the workflow.

Link to tracking issue

Related to #36788

@ArthurSens ArthurSens requested a review from a team as a code owner December 16, 2024 17:58
@ArthurSens ArthurSens requested a review from songy23 December 16, 2024 17:58
@ArthurSens
Copy link
Member Author

/label "Skip changelog"

@songy23 songy23 added ci-cd CI, CD, testing, build issues Skip Changelog PRs that do not require a CHANGELOG.md entry labels Dec 16, 2024
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cc @pjanotti

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It can help in some cases, but it doesn't provide a strong guarantee that we catch any change breaking windows tests. I think we need to enable them on merge queue.

@dmitryax dmitryax merged commit 1f10e2d into open-telemetry:main Dec 16, 2024
183 of 187 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 16, 2024
@ArthurSens ArthurSens deleted the run-windows-tests branch December 16, 2024 20:24
@ArthurSens
Copy link
Member Author

ArthurSens commented Dec 16, 2024

LGTM. It can help in some cases, but it doesn't provide a strong guarantee that we catch any change breaking windows tests. I think we need to enable them on merge queue.

If I understand correctly, merge queues only run workflows marked as required on PRs. That means that we'd need to run windows tests on every PR, right?

@dmitryax
Copy link
Member

If I understand correctly, merge queues only run workflows marked as required on PRs. That means that we'd need to run windows tests on every PR, right?

I'm not sure, but my understanding is that it's not required. We could make Windows tests be required on merge queue but not run them for every PR. @mx-psi @jade-guiton-dd please correct me

@mx-psi
Copy link
Member

mx-psi commented Dec 17, 2024

We could make Windows tests be required on merge queue but not run them for every PR. @mx-psi @jade-guiton-dd please correct me

Arthur is correct that required is an all-or-nothing feature (so, the job would be mark as required in both the merge queue and PRs or in neither) BUT you can skip required jobs, so we could have logic to skip them on PRs

sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…emetry#36857)

#### Description
We've noticed that many of the CI failures on main come from Windows e2e
tests. We want to be more proactive in catching those failures, so this
PR changes the workflow triggering Windows e2e tests to ensure they run
on Pull Requests that change Windows-related files, even if the label
`Run Windows` is absent.

It does so by looking at changed files. If the word `windows` is present
in the file name, it triggers the workflow.

#### Link to tracking issue
Related to
open-telemetry#36788

---------

Signed-off-by: Arthur Silva Sens <[email protected]>
mterhar pushed a commit to mterhar/opentelemetry-collector-contrib that referenced this pull request Dec 19, 2024
…emetry#36857)

#### Description
We've noticed that many of the CI failures on main come from Windows e2e
tests. We want to be more proactive in catching those failures, so this
PR changes the workflow triggering Windows e2e tests to ensure they run
on Pull Requests that change Windows-related files, even if the label
`Run Windows` is absent.

It does so by looking at changed files. If the word `windows` is present
in the file name, it triggers the workflow.

#### Link to tracking issue
Related to
open-telemetry#36788

---------

Signed-off-by: Arthur Silva Sens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues os:windows Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants