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

Migrate to use new parsers #3069

Merged
merged 17 commits into from
Jul 8, 2024
Merged

Conversation

jaronoff97
Copy link
Contributor

Description: Enables the operator to use the new parsers

Link to tracking Issue(s):

Testing: Unit tests

Documentation:

@@ -133,6 +139,53 @@ type Config struct {
Service Service `json:"service" yaml:"service"`
}

// getPortsForComponentTypes gets the ports for the given ComponentType(s).
// NOTE FOR REVIEWERS: we could also do this by introducing something in components that is aware of which retriever
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seeking opinions here

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this depends on where we want to handle finding the right component config. At this level, we know it exists, as we're iterating over the map. The retriever would have to check and potentially return an error. Is there any other place we need this logic in? If it's just here, I'd keep it as is.

apis/v1beta1/config.go Outdated Show resolved Hide resolved
@@ -37,105 +25,3 @@ const (
func (c ComponentType) String() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be deleted in a followup!

@jaronoff97 jaronoff97 marked this pull request as ready for review June 26, 2024 15:48
@jaronoff97 jaronoff97 requested a review from a team June 26, 2024 15:48
apis/v1beta1/config.go Outdated Show resolved Hide resolved
apis/v1beta1/config.go Outdated Show resolved Hide resolved
@@ -133,6 +139,53 @@ type Config struct {
Service Service `json:"service" yaml:"service"`
}

// getPortsForComponentTypes gets the ports for the given ComponentType(s).
// NOTE FOR REVIEWERS: we could also do this by introducing something in components that is aware of which retriever
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this depends on where we want to handle finding the right component config. At this level, we know it exists, as we're iterating over the map. The retriever would have to check and potentially return an error. Is there any other place we need this logic in? If it's just here, I'd keep it as is.

internal/components/component.go Show resolved Hide resolved
)

type ComponentType int
type ComponentKind int
Copy link
Member

Choose a reason for hiding this comment

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

I am just curious why kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was per @swiatekm-sumo comment here #3069 (comment)

@jaronoff97 jaronoff97 requested a review from pavolloffay July 2, 2024 13:38
@pavolloffay
Copy link
Member

@jaronoff97 it is approved, can we merge it?

@jaronoff97 jaronoff97 merged commit 1289c63 into open-telemetry:main Jul 8, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants