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

S3BucketBuildArtifacts ignoring "withIncludeBuildId" value #1347

Closed
wmunyan opened this issue Dec 13, 2018 · 9 comments · Fixed by #1354
Closed

S3BucketBuildArtifacts ignoring "withIncludeBuildId" value #1347

wmunyan opened this issue Dec 13, 2018 · 9 comments · Fixed by #1354
Assignees
@RomainMuller
Labels
@aws-cdk/aws-codebuild Related to AWS CodeBuild bug This issue is a bug. language/java Related to Java bindings

Comments

@wmunyan
Copy link

wmunyan commented Dec 13, 2018 •
edited
Loading

When creating an S3BucketBuildArtifacts instance, using the S3BucketBuildArtifactsProps.builder() methods, specifically the withIncludeBuildId, it doesn't matter if I set the property to true or false. When I execute cdk synth on my Stack, the Artifact always indicates a NamespaceType: BUILD_ID. I'd like that to be "None".

Code snippet:

S3BucketBuildArtifacts codebuildS3BucketBuildArtifacts = new S3BucketBuildArtifacts(
	S3BucketBuildArtifactsProps.builder()
		.withBucket(deploymentBucket)
		.withPath("Workflow-Path")
		.withName("Workflow-Name")
		.withPackageZip(Boolean.FALSE)
		.withIncludeBuildId(Boolean.FALSE)  // doesn't matter what I put here...
		.build())

Thanks for any help/advice.
@RomainMuller RomainMuller added language/java Related to Java bindings @aws-cdk/aws-codebuild Related to AWS CodeBuild labels Dec 13, 2018
@RomainMuller
Copy link
Contributor

I made a minimal reproduction using the sample you provided (thanks for that!) and it looks like there is a bug. Running cdk synth with JSII_DEBUG=1 seems to indicate the includeBuildId property is never queried from the userland, which is unexpected.

@RomainMuller RomainMuller added the bug This issue is a bug. label Dec 13, 2018
@RomainMuller RomainMuller self-assigned this Dec 13, 2018
@RomainMuller
Copy link
Contributor

Found the issue to stem from how the property is named in TypeScript, which causes the dynamic override to be on an other name.

RomainMuller added a commit that referenced this issue Dec 13, 2018
@RomainMuller
fix(codebuild): Rename includeBuildID property of S3BucketBuildArtifacts
890bbd5 
The property name ending with `ID` caused a mismatch of names to appear
in non-TypeScript languages, as the name translation rules would convert
back to `Id`.

BREAKING CHANGE: the `includeBuildID` property of
`S3BucketBuildArtifacts` was renamed to `includeBuildId` (note the
lower-case trailing `d`).

Fixes #1347
RomainMuller added a commit that referenced this issue Dec 13, 2018
@RomainMuller
fix(codebuild): Rename includeBuildID property of S3BucketBuildArtifacts
f475a98 
The property name ending with `ID` caused a mismatch of names to appear
in non-TypeScript languages, as the name translation rules would convert
back to `Id`.

BREAKING CHANGE: the `includeBuildID` property of
`S3BucketBuildArtifacts` was renamed to `includeBuildId` (note the
lower-case trailing `d`).

Fixes #1347
This was referenced Dec 13, 2018
Merged
Closed
@RomainMuller
Copy link
Contributor

Created an issue on JSII side to address the suboptimal behavior of the Java code generation that, if fixed, would have made this issue much faster to identify and fix.

RomainMuller added a commit that referenced this issue Dec 13, 2018
@RomainMuller
fix(codebuild): Rename includeBuildID property of S3BucketBuildArtifa…
84eb7ad 
…cts (#1354)

The property name ending with `ID` caused a mismatch of names to appear
in non-TypeScript languages, as the name translation rules would convert
back to `Id`.

BREAKING CHANGE: the `includeBuildID` property of
`S3BucketBuildArtifacts` was renamed to `includeBuildId` (note the
lower-case trailing `d`).

Fixes #1347
@wmunyan
Copy link
Author

wmunyan commented Jan 3, 2019

Hello,
I see that the fix for this has been included in release 0.21.0. I have referenced the new version in my gradle project, but the issue persists. Should the function name have changed to withIncludeBuildID (in the Builder for S3BucketBuildArtifactsProps)?

Thanks for any help!
Cheers,
-Bill M.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2019

No, the correct name is still withIncludeBuildId.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2019

This is odd.

@rix0rrr rix0rrr reopened this Jan 3, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2019

I would prefer to wait for @RomainMuller to get back from holidays as he has more state on this issue and will be able to diagnose/validate the situation quicker.

@wmunyan
Copy link
Author

wmunyan commented Jan 4, 2019

Hello, sorry about that - This can be closed. My CDK application referenced the new version but when built was still pulling in the old version. Bad IDE'ing on my part. Once corrected, this issue is no longer present.

Thanks for tolerating my dumbassery.
-Bill M.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 4, 2019

No problem, glad to hear it got solved!

@rix0rrr rix0rrr closed this as completed Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RomainMuller RomainMuller

Labels
@aws-cdk/aws-codebuild Related to AWS CodeBuild bug This issue is a bug. language/java Related to Java bindings
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(codebuild): Rename includeBuildID property of S3BucketBuildArtifacts aws/aws-cdk
3 participants
@RomainMuller @rix0rrr @wmunyan