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

[For 2.20] Move ContainerRuntime class to internal scope #23341

Conversation

markfields
Copy link
Member

@markfields markfields commented Dec 17, 2024

Description

Fixes AB#26794
See #23329

Stop exporting ContainerRuntime class under the /legacy path. See deprecation release note

Breaking Changes

This is a breaking change since ContainerRuntime has been exported before. It's deprecated as of 2.12.0, and all known usages in partner codebases have been or are being removed.

@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc changeset-present public api change Changes to a public API labels Dec 17, 2024
@markfields markfields linked an issue Dec 17, 2024 that may be closed by this pull request
@github-actions github-actions bot added the base: main PRs targeted against main branch label Dec 17, 2024
@markfields markfields force-pushed the test/legacy-breaks/client/2.20/containerruntime-internal branch from 5f75fd4 to b84092a Compare January 8, 2025 03:13
@markfields markfields marked this pull request as ready for review January 8, 2025 03:14
@Copilot Copilot bot review requested due to automatic review settings January 8, 2025 03:14
@markfields markfields requested review from a team as code owners January 8, 2025 03:14

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 8 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • packages/runtime/container-runtime/api-report/container-runtime.legacy.alpha.api.md: Evaluated as low risk
  • packages/runtime/container-runtime/src/containerRuntime.ts: Evaluated as low risk
  • packages/framework/aqueduct/src/container-runtime-factories/baseContainerRuntimeFactory.ts: Evaluated as low risk
Comments suppressed due to low confidence (2)

packages/test/test-utils/src/testContainerRuntimeFactory.ts:91

  • [nitpick] The variable name 'runtime' is ambiguous. It should be renamed to 'containerRuntime'.
public async instantiateFirstTime(runtime: ContainerRuntime): Promise<void> {

packages/test/test-utils/src/testContainerRuntimeFactory.ts:109

  • [nitpick] The variable name 'runtime' is ambiguous. It should be renamed to 'containerRuntime'.
public async instantiateFromExisting(runtime: ContainerRuntime): Promise<void> {
.changeset/better-mails-judge.md Outdated Show resolved Hide resolved
.changeset/better-mails-judge.md Outdated Show resolved Hide resolved
markfields and others added 2 commits January 8, 2025 10:49
Co-authored-by: jzaffiro <[email protected]>
It was used to justify breaking the type here.
@@ -121,14 +123,11 @@ export class BaseContainerRuntimeFactory
* Called at the start of initializing a container, to create the container runtime instance.
* @param context - The context for the container being initialized
* @param existing - Whether the container already exists and is being loaded (else it's being created new just now)
*
* @deprecated This function should not be called directly, use instantiateRuntime instead.
Copy link
Member Author

Choose a reason for hiding this comment

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

@microsoft/fluid-cr-api -- I had marked this as deprecated to justify this break. Simplest thing is to remove the deprecation as I add the new type, as you see here.

But taking a step back, I think preInitialize, along with instantiateFirstTime and instantiateFromExisting should be protected not public. Once instantiated, this class (and base class RuntimeFactoryHelper) should only be referenced via the interface IRuntimeFactory, IMO.

If we go that direction I can leave this deprecated and do a follow-up PR to deprecate the others, and then in 2.30 switch them all to protected. Maybe I'm missing something though. Thoughts?

See also the deprecation release note for when I marked this as deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

i would only leave it deprecated if you plan follow up and finish it, otherwise i'd just file a bug, but let the person who takes it on do the deprecation

Copy link
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

🥳

Copy link
Contributor

github-actions bot commented Jan 8, 2025

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170492 links
    1603 destination URLs
    1838 URLs ignored
       0 warnings
       0 errors


@markfields markfields requested a review from jzaffiro January 8, 2025 19:13
Copy link
Contributor

@jzaffiro jzaffiro left a comment

Choose a reason for hiding this comment

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

Approved for docs!

@markfields markfields enabled auto-merge (squash) January 8, 2025 19:29
@markfields markfields merged commit 61ba06a into microsoft:main Jan 8, 2025
38 checks passed
@markfields markfields deleted the test/legacy-breaks/client/2.20/containerruntime-internal branch January 8, 2025 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ContainerRuntime class
3 participants