-
Notifications
You must be signed in to change notification settings - Fork 536
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
Deprecate ContainerRuntime class #23331
Conversation
This needs some follow-up... it's exposed in public (legacy-alpha) function signatures.
And add interface-based signatures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- packages/framework/aqueduct/api-report/aqueduct.legacy.alpha.api.md: Evaluated as low risk
- packages/runtime/container-runtime/api-report/container-runtime.legacy.alpha.api.md: Evaluated as low risk
- examples/utils/migration-tools/src/compositeRuntime/loadCompositeRuntime.ts: Evaluated as low risk
- packages/runtime/container-runtime/src/containerRuntime.ts: Evaluated as low risk
Marking as Release-Blocking to make sure this deprecation announcement makes it into the 2.12 release. |
Co-authored-by: Alex Villarreal <[email protected]> Co-authored-by: Joshua Smithrud <[email protected]>
packages/framework/aqueduct/src/container-runtime-factories/baseContainerRuntimeFactory.ts
Outdated
Show resolved
Hide resolved
@jason-ha, @sonalideshpandemsft I'm going to wait to merge this until we can talk on Monday about what to do about the Loop CI pipeline hitting lint failures. |
packages/framework/aqueduct/src/container-runtime-factories/baseContainerRuntimeFactory.ts
Outdated
Show resolved
Hide resolved
packages/framework/aqueduct/src/container-runtime-factories/baseContainerRuntimeFactory.ts
Outdated
Show resolved
Hide resolved
examples/apps/attributable-map/src/modelContainerRuntimeFactoryWithAttribution.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New plan: deprecate the methods on BaseContainerRuntimeFactory that use ContainerRuntime. Alternative for people (I think there is no one) is to copy the implementation of the class and change the type on them to IContainerRuntime.
For legacy+alpha's that are becoming internal I recommend an alternate pattern that keeps the deprecation to external uses and preserves internal as non-deprecated. See #23332 |
Since you have a plan to deal with all of those in Loop, I would suggest adding lint suppressions in Loop with a comment noting that while the suppressions aren't required with current package they are required for pending update and needed to keep the integration pipeline passing. I do not know how we make a good process generally. |
`BaseContainerRuntimeFactory` has some changes as well, since it exposed `ContainerRuntime` in several function signatures: | ||
|
||
* `instantiateFirstTime` - Takes the wider type `IContainerRuntime` instead of `ContainerRuntime` | ||
* `instantiateFromExisting` - Takes the wider type `IContainerRuntime` instead of `ContainerRuntime` | ||
* `preInitialize` - deprecated as well, since it returns `ContainerRuntime` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseContainerRuntimeFactory
is not @sealed
so right now people could in theory subclass it, and I believe that would make this breaking for anyone that was doing it. I don't know how confident we are that it is not happening today... and if we are confident enough, maybe we should mark it @sealed
. And well, depending on that level of confidence, these might actually need to wait until 2.20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call-out. I discussed this at length with @jason-ha, it's technically not a breaking change based on TypeScript weirdness (kind of disappointing), but you're right that it would lead to unexpected behavior if a subclass was counting on getting ContainerRuntime
and then a caller provided something outside that.
HOWEVER -- in the ecosystem in which this is used, it'll always be ContainerRuntime
. We just don't want to talk about it :)
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Description
Fixes AB#25439. Precursor to #23329.
We intend to mark the class
ContainerRuntime
as@internal
which will give us great freedom to iterate on its internals freely - it is an implementation detail after all! To do so we need to put it on a deprecation path so any few remaining usages in partner code are migrated offContainerRuntime
, like so:IContainerRuntime
(or in rare cases,IRuntime
)ContainerRuntime.loadRuntime
replace it with the free functionloadContainerRuntime
.