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.

Sign up for GitHub

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

Jump to bottom

[cmd/opampsupervisor] Fix windows support #34573

Merged
merged 15 commits into from
Aug 16, 2024
Merged

[cmd/opampsupervisor] Fix windows support #34573

merged 15 commits into from
Aug 16, 2024

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Aug 9, 2024 •
edited
Loading

Description:

  • Fixed windows improper signalling for the agent to shutdown.
  • Fixed not closing files in e2e tests (windows will error if a file is not closed when it attempts to delete the temp directory said file is in)
  • Fixed test config templates quoting (windows filepaths would have trouble since the backslash acts like a character escape)
  • Added a workflow to allow running e2e tests for windows

Link to tracking Issue: Closes #34570

Testing:

  • Existing e2e tests cover this.
  • I've manually tested remotely configuring a windows supervisor with BindPlane.

@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review August 9, 2024 15:23
@BinaryFissionGames BinaryFissionGames requested a review from a team August 9, 2024 15:23
@BinaryFissionGames
Copy link
Contributor Author

This needs the Run Windows label to test out the new workflow for the supervisor windows e2e tests.

@evan-bradley evan-bradley added the Run Windows Enable running windows test on a PR label Aug 9, 2024
tigrannajaryan
tigrannajaryan approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Code LGTM (I don't have a way to verify this on Windows).

@djaglowski djaglowski merged commit eaa0888 into open-telemetry:main Aug 16, 2024
172 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 16, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
@BinaryFissionGames @djaglowski @f7o
[cmd/opampsupervisor] Fix windows support (open-telemetry#34573)
7e94861 
**Description:** <Describe what has changed.>
* Fixed windows improper signalling for the agent to shutdown.
* Fixed not closing files in e2e tests (windows will error if a file is
not closed when it attempts to delete the temp directory said file is
in)
* Fixed test config templates quoting (windows filepaths would have
trouble since the backslash acts like a character escape)
* Added a workflow to allow running e2e tests for windows

**Link to tracking Issue:** Closes open-telemetry#34570

**Testing:** 
* Existing e2e tests cover this.
* I've manually tested remotely configuring a windows supervisor with
BindPlane.

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-bradley evan-bradley evan-bradley left review comments

@pjanotti pjanotti pjanotti approved these changes

@djaglowski djaglowski djaglowski approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

Assignees

@djaglowski djaglowski

Labels
cmd/opampsupervisor os:windows Run Windows Enable running windows test on a PR
Projects
None yet
Milestone
v0.108.0
Development

Successfully merging this pull request may close these issues.

[cmd/opampsupervisor]: Fix supervisor on Windows
6 participants
@BinaryFissionGames @tigrannajaryan @djaglowski @pjanotti @evan-bradley @crobert-1