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.

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

Reconsider exporting symbols from internal modules #17450

Closed
chlowell opened this issue Apr 5, 2022 · 9 comments
Closed

Reconsider exporting symbols from internal modules #17450

chlowell opened this issue Apr 5, 2022 · 9 comments
Assignees
@jhendrixMSFT
Labels
Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation design-discussion An area of design currently under discussion and open to team and community feedback.

Comments

@chlowell
Copy link
Member

chlowell commented Apr 5, 2022

azcore/log exports a type and constants from sdk/internal/log. This instance is low risk but the pattern is troubling in general because it makes sdk/internal not really internal. We should discuss whether to allow this pattern and, if we decide to allow it, what guard rails to place around internal modules (we'd want them around e.g. keyvault/internal as well).
@chlowell chlowell added design-discussion An area of design currently under discussion and open to team and community feedback. blocking-release Blocks release labels Apr 5, 2022
@JeffreyRichter
Copy link
Member

JeffreyRichter commented Apr 5, 2022 via email

@richardpark-msft
Copy link
Member

I have a similar situation in azservicebus - I have a set of constants that represent the receive mode and I use them in both azservicebus/internal and azservicebus code.

@chlowell
Copy link
Member Author

chlowell commented Apr 8, 2022

I think it's okay for a module to re-export things from its own internal subpackages when necessary. I'm concerned about internal modules like sdk/internal. These are nominally private, accessible only to SDK authors who may not know parts of them are functionally public due to being re-exported by some public module.

@chkohner
Copy link

Also, if you're going to export internals, please make sure they are fully exported. Atm, you export azcore.TokenCredential = shared.TokenCredential but not the corresponding TokenRequestOptions used as a parameter (thus making the interface unusable in practice). Also, it's unclear from a consumer perspective what the distinction between azcore and azpolicy is. (Which, btw, azpolicy.TokenRequestOptions IS exporting the shared internal atm, where azcore is not...)

@chkohner
Copy link

As an additional note, if you're considering NOT exporting key internals at all, that'd be really unfortunate. If they are exported (correctly), I can at least leverage key components (e.g. azidentity for managed identities) to make generic REST calls to Azure endpoints that don't have great Go SDK support yet.

@jhendrixMSFT
Copy link
Member

I agree that azcore shouldn't be exporting content from internal. I think some of this design predates when we refactored azcore. The only wrinkle here is how to handle Should(), Write(), and Writef(). The current implementation permits only SDK authors to call these funcs as they live in internal which at some point we thought was desirable. How much do we really care about that?

@JeffreyRichter
Copy link
Member

These are for logging, right? If so, I think it still best that only client lib authores beable to call them. Our logging infrastrutue is for logging client lib operations and is not designed as a general purpose logging mechanism for customers to use themselves.

@jhendrixMSFT
Copy link
Member

Correct, this is just for logging.

In essence, the internal/log.Event type is an extensible enum type, intended to be extended by SDK authors, and used by customers when programmatically selecting a subset of events to log.

Its APIs are split into two buckets. One is intended to be called by customers, and the other to be called by SDK authors. In order to enforce this dichotomy, the buckets are implemented in azcore/log and internal/log respectively.

The bucketing of the APIs requires the Event type to be defined in internal/log so it's consumable in both modules. And since Event participates in customer-facing APIs, it must be exported from azcore/log which puts us into this weird place.

This was referenced Apr 25, 2022
Merged
Merged
Merged
@jhendrixMSFT
Copy link
Member

These have been cleaned up and will be included in the azcore@v0.24.0 release.

@RickWinter RickWinter added CodeGen Issues that relate to code generation Client This issue points to a problem in the data-plane of the library. and removed blocking-release Blocks release labels Apr 29, 2022
@zacharyljones zacharyljones mentioned this issue Jul 14, 2022
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jhendrixMSFT jhendrixMSFT

Labels
Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation design-discussion An area of design currently under discussion and open to team and community feedback.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@JeffreyRichter @RickWinter @jhendrixMSFT @chlowell @richardpark-msft @chkohner @seankane-msft