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

Add internal localhostgate package #30774

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jan 25, 2024

Description:

Follow up to open-telemetry/opentelemetry-collector/pull/8622

Link to tracking Issue: First step of #30702

@mx-psi mx-psi requested review from a team, songy23 and atoulme January 25, 2024 12:18
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Should there be a changelog or a README for the feature gate?

@mx-psi
Copy link
Member Author

mx-psi commented Jan 25, 2024

Should there be a changelog or a README for the feature gate?

I was thinking we can do a changelog item once we actually use it in individual components, listing all the components we are using this in. I can also add the changelog item now with an empty list of components if you think that's better

@songy23
Copy link
Member

songy23 commented Jan 25, 2024

Yes I think it makes sense to start a list now and expand it when changes are made to each component.

@crobert-1
Copy link
Member

I'm wondering if there's a good way to enforce keeping this up to date core. There will be times they differ, but it should only be temporarily. There should be a PR opened for both repositories, one will just get merged before the other.

It could be a CI check that simply fails whenever a user changes the contents of these files, warning the user that core needs updated as well. A triager, approver, or maintainer could then come, confirm a PR is also opened against core, and then add some kind of skip label that ignores the check? Or just approve and merge it with the failing CI action knowing it's not an issue. If it's a CI action it would also need to be in core to make sure contrib stays up to date as well.

Otherwise we could add a CI action that simply compared file contents and fails when they don't match. This would be a bit more involved and would fail in unrelated PRs, which would be confusing.

I don't want to make it too complicated but speaking from experience, a comment at the top of a file is pretty easy to miss.

@mx-psi mx-psi changed the title [chore] Add internal localhostgate package Add internal localhostgate package Jan 26, 2024
@mx-psi mx-psi requested a review from songy23 January 26, 2024 12:55
@mx-psi
Copy link
Member Author

mx-psi commented Jan 26, 2024

@crobert-1 I don't expect much to change here, but it makes sense to have something like this. I filed #30793 to do this, but I would like to do it separately (I want to unblock PRs that start to use this feature gate in components)

@mx-psi mx-psi requested a review from crobert-1 January 26, 2024 12:56
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Needs make tidy

@mx-psi mx-psi merged commit 5a10c4b into open-telemetry:main Jan 29, 2024
85 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 29, 2024
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
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.

4 participants