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

[mdatagen] add unsupported_platforms support and enable several components #30640

Merged
merged 4 commits into from
Jan 20, 2024
Merged

[mdatagen] add unsupported_platforms support and enable several components #30640

merged 4 commits into from
Jan 20, 2024

Conversation

fatsheep9146
Copy link
Contributor

@fatsheep9146 fatsheep9146 commented Jan 17, 2024 •
edited
Loading

Description:
Add supported_platforms to enable components to describe which platforms are supported, meanwhile this configuration can also be used to generate the liftcycle test to skip windows when the component does not support windows.

Link to tracking Issue:
fix #30561

Testing:

Documentation:

@fatsheep9146 fatsheep9146 requested a review from a team January 17, 2024 08:46
@fatsheep9146 fatsheep9146 added Run Windows Enable running windows test on a PR and removed cmd/mdatagen mdatagen command receiver/podman labels Jan 17, 2024
@fatsheep9146 fatsheep9146 changed the title [mdatagen] add supported_platforms configuration & enable podman receiver WIP [mdatagen] add supported_platforms configuration & enable podman receiver Jan 17, 2024
@fatsheep9146 fatsheep9146 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 17, 2024
@fatsheep9146 fatsheep9146 changed the title WIP [mdatagen] add supported_platforms configuration & enable podman receiver WIP [mdatagen] add supported_platforms configuration & enable several components Jan 17, 2024
@fatsheep9146 fatsheep9146 changed the title WIP [mdatagen] add supported_platforms configuration & enable several components [mdatagen] add supported_platforms configuration & enable several components Jan 17, 2024
@fatsheep9146 fatsheep9146 changed the title [mdatagen] add supported_platforms configuration & enable several components [mdatagen] add supported_platforms support and enable several components Jan 17, 2024
@fatsheep9146 fatsheep9146 mentioned this pull request Jan 17, 2024
Closed
dmitryax
dmitryax reviewed Jan 17, 2024
View reviewed changes
testbed/testbed/child_process_collector.go Outdated Show resolved Hide resolved
cmd/mdatagen/main.go Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

Also I don't think we need to render +build. I started removal of this directive in open-telemetry/opentelemetry-collector#9304

@djaglowski djaglowski mentioned this pull request Jan 19, 2024
Merged
@djaglowski djaglowski mentioned this pull request Jan 19, 2024
Merged
@fatsheep9146
[mdatagen] add unsupported_platforms configuration
dc27578 
Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
@fatsheep9146 fatsheep9146 changed the title [mdatagen] add supported_platforms support and enable several components [mdatagen] add unsupported_platforms support and enable several components Jan 19, 2024
codeboten
codeboten reviewed Jan 19, 2024
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @fatsheep9146! This looks great, just one comment

cmd/opampsupervisor/supervisor/supervisor_test.go
@@ -52,6 +53,7 @@ func Test_composeEffectiveConfig(t *testing.T) {

expectedConfig, err := os.ReadFile("../testdata/collector/effective_config.yaml")
require.NoError(t, err)
expectedConfig = bytes.ReplaceAll(expectedConfig, []byte("\r\n"), []byte("\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

can this change be made in a separate PR? it seems unrelated to the change here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#30690 is created @codeboten

@pjanotti
Copy link
Contributor

this configuration can also be used to generate the liftcycle test to skip windows when the component does not support windows

My understanding is that we should be testing all components on all OSes giving that we expect proper error messages and the that the collector configuration can be parsed on different platforms. Not testing the lifecycle on such platforms as of recently risks crashes without proper error message and regressions in this regard, see:

pjanotti
pjanotti suggested changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

I understand this as an emergency fix to the broken Windows CI, however, in the end the tests should keep loading the config and expecting a definite error when the platform is not supported.

@fatsheep9146
Copy link
Contributor Author

fatsheep9146 commented Jan 20, 2024 •
edited
Loading

I understand this as an emergency fix to the broken Windows CI, however, in the end the tests should keep loading the config and expecting a definite error when the platform is not supported.

Yes, so @dmitryax suggested:

Let's start with unsupported_platforms option in metadata.yaml and just list them in go:build pragma

I think we can improve this as you suggest in following prs.
@pjanotti
I will create an issue to track this.

fatsheep9146 and others added 3 commits January 20, 2024 08:41
@fatsheep9146 @dmitryax
Update cmd/mdatagen/templates/component_test.go.tmpl
ec378e1 
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@fatsheep9146 @dmitryax
Update cmd/mdatagen/templates/component_test.go.tmpl
4a57913 
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@fatsheep9146
regenerate the podreceiver and sshreceiver
59fa59c 
Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
@fatsheep9146 fatsheep9146 mentioned this pull request Jan 20, 2024
Closed
@fatsheep9146
Copy link
Contributor Author

I understand this as an emergency fix to the broken Windows CI, however, in the end the tests should keep loading the config and expecting a definite error when the platform is not supported.

Yes, so @dmitryax suggested:

Let's start with unsupported_platforms option in metadata.yaml and just list them in go:build pragma

I think we can improve this as you suggest in following prs. @pjanotti I will create an issue to track this.

#30691 is created @pjanotti

@dmitryax
Copy link
Member

Thanks @fatsheep9146

@dmitryax dmitryax merged commit ff3023c into open-telemetry:main Jan 20, 2024
93 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 20, 2024
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
@fatsheep9146 @dmitryax @cparkins
[mdatagen] add unsupported_platforms support and enable several com…
02dd7af 
…ponents (open-telemetry#30640)

**Description:**
Add `supported_platforms` to enable components to describe which
platforms are supported, meanwhile this configuration can also be used
to generate the liftcycle test to skip windows when the component does
not support windows.

**Link to tracking Issue:** fix open-telemetry#30561

---------

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten left review comments

@pjanotti pjanotti pjanotti requested changes

@dmitryax dmitryax dmitryax approved these changes

@evan-bradley evan-bradley Awaiting requested review from evan-bradley evan-bradley is a code owner

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

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

@nslaughter nslaughter Awaiting requested review from nslaughter

@rogercoll rogercoll Awaiting requested review from rogercoll

Assignees

@mx-psi mx-psi

Labels
cmd/mdatagen mdatagen command cmd/opampsupervisor receiver/podman receiver/sshcheck Run Windows Enable running windows test on a PR Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.93.0
Development

Successfully merging this pull request may close these issues.

[receiver/podman] lifecycle test failure on windows
5 participants
@fatsheep9146 @dmitryax @pjanotti @codeboten @mx-psi