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

feat: upgrade to cdk v2 #1074

Merged
merged 9 commits into from
May 3, 2022
Merged

feat: upgrade to cdk v2 #1074

merged 9 commits into from
May 3, 2022

Conversation

corymhall
Copy link
Contributor

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.

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
Copy link
Contributor Author

@corymhall corymhall left a 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),
Copy link
Contributor Author

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"
Copy link
Contributor Author

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}`;
Copy link
Contributor Author

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: {
Copy link
Contributor Author

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');
Copy link
Contributor Author

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' }],
Copy link
Contributor Author

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
Copy link
Contributor Author

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) => {
Copy link
Contributor Author

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;
Copy link
Contributor Author

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',
Copy link
Contributor Author

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.

@corymhall corymhall requested a review from a team January 25, 2022 20:49
Copy link
Contributor

@rix0rrr rix0rrr left a 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

@corymhall
Copy link
Contributor Author

I'm okay with it, but needs a major version bump

Shouldn't the major version bump happen automatically because of the BREAKING CHANGE?

@github-actions
Copy link

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.

@github-actions github-actions bot added the stale label Feb 11, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot closed this Feb 13, 2022
@kaizencc kaizencc reopened this Apr 7, 2022
@kaizencc
Copy link
Contributor

kaizencc commented Apr 7, 2022

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 BREAKING CHANGE tag has to happen at the end of the PR description. Not sure if that's a necessity or just convention.

@corymhall I'm happy to approve this if you take a quick look and make sure things are still up to snuff.

@kaizencc kaizencc removed the stale label Apr 7, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale label Apr 22, 2022
@Chriscbr Chriscbr removed the stale label Apr 22, 2022
@mergify mergify bot merged commit b252f01 into cdklabs:main May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants