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

fix(ecs): update driverOpts type definition from array to map #3358

Merged
merged 11 commits into from
Aug 9, 2019

Conversation

nmussy
Copy link
Contributor

@nmussy nmussy commented Jul 19, 2019

BREAKING CHANGE @aws-cdk/aws-ecs.DockerVolumeConfiguration.driverOpts: The type was incorrectly defined as string[], now fixed to {[key: string]: string}.

Fixes #3341


Please read the contribution guidelines and follow the pull-request checklist.

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

@nmussy nmussy requested a review from a team as a code owner July 19, 2019 13:43
@nmussy
Copy link
Contributor Author

nmussy commented Jul 19, 2019

I don't understand why the build fails, could someone help me out? Thanks in advance.

@eladb
Copy link
Contributor

eladb commented Jul 21, 2019

I suspect this is a breaking change although seems like the original type was actually a bug. @SoManyHs what do you think?

@piradeepk
Copy link
Contributor

I suspect this is a breaking change although seems like the original type was actually a bug. @SoManyHs what do you think?

@eladb Yes to both, the original type was incorrect, and it should have been a map not an array. @nmussy Thanks for taking the time to fix the issue!

piradeepk
piradeepk previously approved these changes Jul 29, 2019
Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing the issue!

@piradeepk piradeepk changed the title fix(ecs): driverOpts type definition fix(ecs): driverOpts type definition from array to map Jul 29, 2019
@piradeepk piradeepk changed the title fix(ecs): driverOpts type definition from array to map fix(ecs): update driverOpts type definition from array to map Jul 29, 2019
@nmussy
Copy link
Contributor Author

nmussy commented Jul 29, 2019

Thanks for the review! I still don't know why the build fails though?

@piradeepk
Copy link
Contributor

I believe it's because there are changes that break the current behaviour (which is expected). @eladb how do you want to proceed?

@eladb
Copy link
Contributor

eladb commented Jul 31, 2019

Since this is an experimental module, you can add an exception. See aecaa5d

Make sure to ibdicate this in the commit message per convention.

@nmussy nmussy requested a review from SoManyHs as a code owner July 31, 2019 15:57
@nmussy
Copy link
Contributor Author

nmussy commented Jul 31, 2019

Should be good. @eladb Should we tag v2 issue, or at least add a review allowed-breaking-changes.txt step?

@piradeepk
Copy link
Contributor

Since this is an experimental module, you can add an exception. See aecaa5d

Make sure to ibdicate this in the commit message per convention.

@eladb not sure if you're thinking about aws-ecs-patterns but these changes are for the aws-ecs module which should be stable not experimental.

@nmussy
Copy link
Contributor Author

nmussy commented Jul 31, 2019

I ran scripts/check-api-compatibility.sh locally and didn't get any errors. I guess I didn't build it properly. Do I have to add every method listed in the error to the breaking-changes.txt?

changed-type:@aws-cdk/aws-ecs.DockerVolumeConfiguration.driverOpts

@piradeepk piradeepk added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Aug 6, 2019
@mergify mergify bot merged commit 65e4a5d into aws:master Aug 9, 2019
@nmussy nmussy deleted the fix-driver-opts-typing branch August 9, 2019 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECS: Incorrect volume driverOpts typing
3 participants