Skip to content

chore(core): remove all references to BundlingDockerImage in the public API#13814

Merged
mergify[bot] merged 3 commits intomasterfrom
nija-at/bundling-docker-image
Mar 26, 2021
Merged

chore(core): remove all references to BundlingDockerImage in the public API#13814
mergify[bot] merged 3 commits intomasterfrom
nija-at/bundling-docker-image

Conversation

@nija-at
Copy link
Copy Markdown
Contributor

@nija-at nija-at commented Mar 26, 2021

A previous commit - ad01099 - deprecated BundlingDockerImage in favour
of DockerImage.

However, there are still uses of BundlingDockerImage that remain. Since
bundling is still experimental, swap all uses of BundlingDockerImage and
replace with DockerImage.

Motivation
No non-deprecated public API can reference a deprecated type as part of
CDKv2.

BREAKING CHANGE: The type of the image property in BundlingOptions
is changed from BundlingDockerImage to DockerImage.

  • core: The return type of the DockerImage.fromBuild() API is
    changed from BundlingDockerImage to DockerImage.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

A previous commit - ad01099 - deprecated BundlingDockerImage in favour
of DockerImage.

However, there are still uses of BundlingDockerImage that remain. Since
bundling is still experimental, swap all uses of BundlingDockerImage and
replace with DockerImage.

Motivation
No non-deprecated public API can reference a deprecated type as part of
CDKv2.

BREAKING CHANGE: The type of the `image` property in `BundlingOptions`
is changed from `BundlingDockerImage` to `DockerImage`.
* **core:** The return type of the `DockerImage.fromBuild()` API is
changed from `BundlingDockerImage` to `DockerImage`.
@nija-at nija-at requested review from a team and eladb March 26, 2021 11:03
@nija-at nija-at self-assigned this Mar 26, 2021
@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Mar 26, 2021

@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Mar 26, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 26, 2021
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 26, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 0545e08
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 26, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 9cceb3f into master Mar 26, 2021
@mergify mergify bot deleted the nija-at/bundling-docker-image branch March 26, 2021 13:14
// timestamps.
const hash = FileSystem.fingerprint(path, { extraHash: JSON.stringify(options) });
return new BundlingDockerImage(tag, hash);
DockerImage.fromBuild(path, options) as BundlingDockerImage;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's doing on here? Seems like the return statement is missing and also the downcast seems hazardous.

@jogold any comments?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original API returned BundlingDockerImage, hence returning the same to keep it backwards compat.

The alternative is to mark the API return type in the method signature and let typescript do the downcast.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am confused. This code does not return anything (there is no return statement). I don't see how this alternative can safely work because fromBuild does not return a BuildingDockerImage, is it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, it should have a return statement. That's a miss. I'll submit a fix.

fromBuild() returns a DockerImage which is a subtype of BundlingDockerImage, so downcasting should be fine, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Mar 31, 2021
…ic API (aws#13814)

A previous commit - ad01099 - deprecated BundlingDockerImage in favour
of DockerImage.

However, there are still uses of BundlingDockerImage that remain. Since
bundling is still experimental, swap all uses of BundlingDockerImage and
replace with DockerImage.

Motivation
No non-deprecated public API can reference a deprecated type as part of
CDKv2.

BREAKING CHANGE: The type of the `image` property in `BundlingOptions`
is changed from `BundlingDockerImage` to `DockerImage`.
* **core:** The return type of the `DockerImage.fromBuild()` API is
changed from `BundlingDockerImage` to `DockerImage`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…ic API (aws#13814)

A previous commit - ad01099 - deprecated BundlingDockerImage in favour
of DockerImage.

However, there are still uses of BundlingDockerImage that remain. Since
bundling is still experimental, swap all uses of BundlingDockerImage and
replace with DockerImage.

Motivation
No non-deprecated public API can reference a deprecated type as part of
CDKv2.

BREAKING CHANGE: The type of the `image` property in `BundlingOptions`
is changed from `BundlingDockerImage` to `DockerImage`.
* **core:** The return type of the `DockerImage.fromBuild()` API is
changed from `BundlingDockerImage` to `DockerImage`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@aws-cdk/core Related to core CDK functionality contribution/core This is a PR that came from AWS.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants