Skip to content

Conversation

@antkmsft
Copy link
Member

@antkmsft antkmsft commented Nov 1, 2024

No description provided.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Nov 1, 2024

API change check

API changes are not detected in this pull request.

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Tbh, this seems to be a bit of unnecessary code churn, for not much value here.

If we don't have a really specific reason (or a clearly better alternative), I would consider not renaming.

@ahsonkhan
Copy link
Contributor

API changes are not detected in this pull request.

@LarryOsterman - changes to types within the _internal namespace are technically breaking (even if end customers don't use it), since older versions of upstream packages within the repo might not build when version mismatch happen between them and core.

Does this API change check take _internal into account (I am assuming it's based on APIView for it's detection)?

@RickWinter
Copy link
Member

API changes are not detected in this pull request.

@LarryOsterman - changes to types within the _internal namespace are technically breaking (even if end customers don't use it), since older versions of upstream packages within the repo might not build when version mismatch happen between them and core.

Does this API change check take _internal into account (I am assuming it's based on APIView for it's detection)?

This class has never shipped from _internal. It was previously in _detail. Now that its moving to _internal we will need to live with the name forever.

@ahsonkhan
Copy link
Contributor

ahsonkhan commented Nov 1, 2024

This class has never shipped from _internal. It was previously in _detail.

That makes sense.
I would have imagined adding a "new" type to an internal or public namespace would be considered an API change, as in an addition (which is what this is doing, going from _detail to _internal). Does this bot only check for "breaking changes" (like renames, moves, deletes)?

@LarryOsterman
Copy link
Member

This class has never shipped from _internal. It was previously in _detail.

That makes sense. I would have imagined adding a "new" type to an internal or public namespace would be considered an API change, as in an addition (which is what this is doing, going from _detail to _internal). Does this bot only check for "breaking changes"?

What "bot"? Are you talking about the API change detection? It looks differences in the API View. We can mark elements (like comments) as "ignore from diff", but if the API shape changes, it will show up.

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Other than making a call on user_agent.cpp one way or another, LGTM.

@ahsonkhan
Copy link
Contributor

Do you still see value or need in keeping this changelog entry, or can we remove it? It seems a bit superfluous.

https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core/CHANGELOG.md#other-changes

Added internal support for the SDK packages to access more of telemetry features.

@antkmsft
Copy link
Member Author

antkmsft commented Nov 1, 2024

@ahsonkhan, I'd keep it. If there is something wrong around the area of telemetry, we and users will know more or less where to look into. Also, if users care/don't care much about any of the listed features, they can chose to upgrade/not upgrade.
The change is of low-lit already by being in the Other Changes section.

@antkmsft antkmsft merged commit 0180c8e into Azure:main Nov 1, 2024
76 checks passed
@antkmsft antkmsft deleted the user-agent-helper branch November 1, 2024 21:07
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.

5 participants