Move ID from Config to CreateSettings for Exporter #3840

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

Closed

bogdandrutu wants to merge 1 commit into open-telemetry:main from bogdandrutu:mvid

Member

bogdandrutu commented Aug 17, 2021

Pros:

The config.Exporter becomes now immutable;
The obsreport needs only settings
The exporter helper does not need config (will be removed in followup). Since config is opaque anyway.

Signed-off-by: Bogdan Drutu [email protected]

bogdandrutu requested review from alolita and Aneurysm9 as code owners

August 17, 2021 00:27

bogdandrutu requested a review from a team

August 17, 2021 00:27

tigrannajaryan requested changes

View reviewed changes

Member

tigrannajaryan left a comment

This has some implications which I am not completely certain about yet and which may prevent some interesting new functionality I was planning for config. I am on PTO until Auth 28, can we wait until then with this change, please?


          Move ID from Config to CreateSettings for Exporter

e968f0d

Pros:
* The config.Exporter becomes now immutable;
* The obsreport needs only settings
* The exporter helper does not need config (will be removed in followup). Since config is opaque anyway.

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu force-pushed the mvid branch from 4a290a0 to e968f0d Compare

August 17, 2021 19:12

Contributor

github-actions bot commented Aug 25, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Sep 2, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Sep 11, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Member Author

bogdandrutu commented Sep 13, 2021

@tigrannajaryan I think we should discuss your concerns.

Contributor

github-actions bot commented Sep 21, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Sep 29, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Oct 7, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Oct 19, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Oct 30, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Nov 11, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Nov 20, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

codeboten reviewed

View reviewed changes

Contributor

codeboten left a comment

@tigrannajaryan @bogdandrutu what was the result of the followup discussion around this PR?

Member

tigrannajaryan commented Nov 29, 2021

@bogdandrutu I don't remember if we discussed this. Do you? I think this looks fine, shouldn't cause problems that I was worried about.

Contributor

github-actions bot commented Dec 7, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Dec 16, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

Contributor

github-actions bot commented Dec 31, 2021

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

github-actions bot added the Stale label

bogdandrutu removed the Stale label

codeboten reviewed

View reviewed changes

Contributor

codeboten left a comment

@bogdandrutu are you planning on revisiting this PR or should it be closed?

bogdandrutu mentioned this pull request

"immutable" config.[Receiver|Processor|Exporter|Extension] interface #4714

Closed

Contributor

github-actions bot commented Jan 26, 2022

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

github-actions bot added the Stale label

Contributor

github-actions bot commented Feb 10, 2022

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions bot closed this

bogdandrutu deleted the mvid branch

July 25, 2022 17:22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels