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

Standarize Settings, Params and Parameters in Receiver #3167

Conversation

pmatyjasek-sumo
Copy link
Contributor

@pmatyjasek-sumo pmatyjasek-sumo commented May 13, 2021

Description:
Continuation work from #2991
Fixes #2650
Replace ReceiverCreateParams with ReceiverCreateSettings.
Replace all dependencies in Receivers.

Link to tracking Issue:
#2650

Testing:
Unit tests

Documentation:

@pmatyjasek-sumo pmatyjasek-sumo requested a review from a team May 13, 2021 13:47
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 21, 2021
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_receivers branch from ba163ec to f78a9e3 Compare May 24, 2021 13:25
@bogdandrutu bogdandrutu removed the Stale label May 24, 2021
@@ -33,17 +33,17 @@ func TestNewNopReceiverFactory(t *testing.T) {
cfg := factory.CreateDefaultConfig()
assert.Equal(t, &nopReceiverConfig{ReceiverSettings: config.NewReceiverSettings(config.NewID("nop"))}, cfg)

traces, err := factory.CreateTracesReceiver(context.Background(), component.ReceiverCreateParams{}, cfg, consumertest.NewNop())
traces, err := factory.CreateTracesReceiver(context.Background(), component.ReceiverCreateSettings{}, cfg, consumertest.NewNop())
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as the other test, it may be useful to have a nop instance. Not a biggie though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I didn't want to change any logic there. Only refactor the code

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Please rebase

@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_receivers branch from f78a9e3 to aa553d7 Compare May 27, 2021 12:51
@pmatyjasek-sumo pmatyjasek-sumo requested a review from alolita as a code owner May 27, 2021 12:51
@bogdandrutu
Copy link
Member

Lint errors

@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_receivers branch from aa553d7 to b777fc7 Compare May 31, 2021 09:17
@bogdandrutu
Copy link
Member

Failing tests not fixed.

@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_receivers branch from b777fc7 to e506652 Compare June 2, 2021 12:01
@pmatyjasek-sumo
Copy link
Contributor Author

Rebased and tests fixed

@bogdandrutu
Copy link
Member

Sorry, merged another one of our PRs and a rebase is needed again :)

@pmatyjasek-sumo
Copy link
Contributor Author

ok :) I'll do it again

Replace all dependencies in Receivers.

Signed-off-by: Patryk Matyjasek <[email protected]>

# Conflicts:
#	component/receiver.go

# Conflicts:
#	exporter/opencensusexporter/opencensus_test.go
#	exporter/prometheusexporter/end_to_end_test.go
Signed-off-by: Patryk Matyjasek <[email protected]>

# Conflicts:
#	CHANGELOG.md

# Conflicts:
#	CHANGELOG.md

# Conflicts:
#	CHANGELOG.md
Signed-off-by: Patryk Matyjasek <[email protected]>
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_receivers branch from e506652 to 585d38f Compare June 2, 2021 13:28
@bogdandrutu bogdandrutu merged commit 86d4088 into open-telemetry:main Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize on params vs settings vs options
3 participants