-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: upgrade to cdk v2 #1074
feat: upgrade to cdk v2 #1074
Conversation
This PR upgrades delivlib to use CDK v2. It also migrates the tests to use the `assertions` library. BREAKING CHANGE: delivlib now requires cdk v2 re #1841
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.
Couple of explanations.
@@ -161,7 +162,7 @@ export class PublishToNpmProject extends Construct implements IPublisher { | |||
const access = props.access ?? NpmAccess.PUBLIC; | |||
|
|||
const shellable = new Shellable(this, 'Default', { | |||
platform: new LinuxPlatform(cbuild.LinuxBuildImage.UBUNTU_14_04_NODEJS_10_1_0), | |||
platform: new LinuxPlatform(cbuild.LinuxBuildImage.STANDARD_5_0), |
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.
Not sure if there are any unintended consequences of this update. STANDARD_5_0
has the tools (node/python, etc) installed already.
GPG_PASSPHRASE_FROM_STDIN="${GPG_PASSPHRASE_FROM_STDIN} --pinentry-mode loopback" | ||
fi | ||
export GPG_PASSPHRASE_FROM_STDIN | ||
export GPG_PASSPHRASE_FROM_STDIN="--passphrase-fd 0 --pinentry-mode loopback" |
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.
I was getting the below error when running the integration tests and providing --pinentry-mode loopback
fixed it. Not sure if it was just caused by differences between the build images or something else.
Running: /tmp/scriptdir/sign-files.sh /tmp/tmp.xdJbThDFY3/jsii-sample-1.5.9.zip
Signing /tmp/tmp.xdJbThDFY3/jsii-sample-1.5.9.zip...
gpg: signing failed: No such file or directory
gpg: signing failed: No such file or directory
@@ -258,7 +259,7 @@ export class Shellable extends Construct { | |||
path: props.scriptDirectory, | |||
}); | |||
|
|||
this.outputArtifactName = `Artifact_${this.node.uniqueId}`; | |||
this.outputArtifactName = `Artifact_${this.node.addr}`; |
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.
In our docs we say that node.addr
is the replacement for node.uniqueId
. See my other comment on the integration results for what effect his has.
const stack = new Stack(); | ||
const app = new App(); | ||
const stack = new Stack(app, 'Default', { | ||
env: { |
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.
If we do not specify the environment now MirrorSource.fromDir
will not be able to resolve the asset bucket name and line 226 (aws s3 cp s3:.* myrepository.zip
) will contain a Token.
const ecrRegistry = 'myregistry'; | ||
const source = MirrorSource.fromDirectory(path.join(__dirname, 'docker-asset'), 'myrepository'); | ||
const source = MirrorSource.fromDir(path.join(__dirname, 'docker-asset'), 'myrepository'); |
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.
fromDirectory
is deprecated.
InputArtifacts: [{ Name: 'Artifact_TestStackPipelineSecondStep91726C11' }], | ||
Match.objectLike({ | ||
ActionTypeId: { Category: 'Build', Owner: 'AWS', Provider: 'CodeBuild', Version: '1' }, | ||
InputArtifacts: [{ Name: 'Artifact_c81eddcbe9657bd312a728fb13df77bc09f9a519b4' }], |
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.
The artifact name change is what we get when we go from node.uniqueId
=> node.addr
@@ -568,7 +234,7 @@ Resources: | |||
- Name: Artifact_Build_Build | |||
Name: TestHelloLinux | |||
OutputArtifacts: | |||
- Name: Artifact_delivlibtestCodeCommitPipelineHelloLinux01786D9D | |||
- Name: Artifact_c883e6647f907b1eb255846397acb348c18b48b3a2 |
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.
The artifact name change is what we get when we go from node.uniqueId
=> node.addr
jest.mock('https'); | ||
|
||
const mockHttpsWrite = jest.fn(); | ||
(https as any).request = jest.fn((_url, _options, cb) => { |
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.
One of the dependency updates did not like (https as any)
jest.mock('../../change-control-lambda/disable-transition'); | ||
jest.mock('../../change-control-lambda/time-window'); | ||
|
||
const mockGetObject = jest.fn().mockName('AWS.S3.getObject'); | ||
(AWS as any).S3 = jest.fn(() => ({ getObject: mockGetObject })).mockName('AWS.S3') as any; |
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.
One of the dependency updates did not like (AWS as any)
so I updated the test.
], | ||
], | ||
}, | ||
Value: '3d34b07ba871989d030649c646b3096ba7c78ca531897bcdb0670774d2f9d3e4.zip', |
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.
This is probably going to happen in aws-cdk
when we start running tests for v2.
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.
I'm okay with it, but needs a major version bump
Shouldn't the major version bump happen automatically because of the |
This pull request is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. If you wish to exclude this issue from being marked as stale, add the "backlog" label. |
Closing this pull request as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen. If you wish to exclude this issue from being marked as stale, add the "backlog" label. |
Whoops, this doesn't look like it should have gone stale. I agree the major version bump will happen automatically, because its a projen managed repo. I'm not exactly sure how projen does this, but I remember seeing somewhere that the @corymhall I'm happy to approve this if you take a quick look and make sure things are still up to snuff. |
This pull request is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. If you wish to exclude this issue from being marked as stale, add the "backlog" label. |
This PR upgrades delivlib to use CDK v2. It also migrates the tests to
use the
assertions
library.BREAKING CHANGE: delivlib now requires cdk v2
re #1841
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.