Skip to content

Conversation

@kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Nov 18, 2021

  • Adds rewriteImports and helper functions rewriteReadmeImports and rewriteMonoPackageImports to aws-cdk-migration package.
  • individual-package-gen consumes rewriteReadmeImports for alpha modules.
  • individual-package-gen treats rosetta fixtures as source files and rewrites imports there as well.
  • Adds aws-cdk-migration as a dependency to ubergen.
  • Refactors rewriteImports in ubergen to use the API in aws-cdk-migration. Has the side affect of removing the string-replace function that was introduced in this PR. This changes the readme import style from import { blah as blah } from 'aws-cdk-lib'; to import * as blah from 'aws-cdk-lib/blah';.
  • fixes error in cfnspec that generated invalid import names for L1 module readmes.
  • fixes bug in cdk-build that only executed one post command.
  • fixes verify-imports scripts typo introduced previously (that was not caught due to the cdk-build bug).

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

@kaizencc kaizencc requested a review from rix0rrr November 18, 2021 21:32
@gitpod-io
Copy link

gitpod-io bot commented Nov 18, 2021

@kaizencc kaizencc marked this pull request as draft November 18, 2021 21:32
@github-actions github-actions bot added the aws-cdk-lib Related to the aws-cdk-lib package label Nov 18, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 18, 2021
@kaizencc
Copy link
Contributor Author

There is a lot going on in this PR and some of it likely does not belong in the same PR together. Especially because the origin of this PR is v2-main and not master. That is why it is marked as draft; hoping we can talk about what makes sense and what doesn't here and then decide on what/how to merge.

@kaizencc
Copy link
Contributor Author

Couple of things @rix0rrr:

  • build breaks because the verify-readme-imports script in aws-cdk-lib is very unhappy as the readme import styles have changed. We can either remove that check, return the original string-replace stuff into ubergen, or refactor code in aws-cdk-lib to produce the imports we want to see in readmes (i looked at this option and it seems doable, but non-trivial).
  • i am sure you have plenty more opinions on this PR so I have not yet rebased to master.
  • i reverted all code in @monocdk-experimental/rewrite-imports (since its no longer used by ubergen. I am not even sure if that is currently used anywhere in the CDK. might be possible to delete?

'',
'```ts',
`import ${module.moduleName.toLocaleLowerCase()} = require('${module.packageName}');`,
`import * as ${module.moduleName.toLocaleLowerCase().replace(/-/g, '_')} from '${module.packageName}';`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be the same old, same old "guess import name from module name" function that we have in a bunch of places? Might also be centralized here...

I guess this will do, but it's perhaps worth considering

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 think it might be overkill to add it here, since this is just a suggested name and can really be anything at this point. No need to add the module function from generated-examples, which is what I think you're referring to. Instead, I have added a more robust regex to ensure that the module name is always valid.

@kaizencc kaizencc added the pr/do-not-merge This PR should not be merged at this time. label Nov 22, 2021
@kaizencc
Copy link
Contributor Author

@Mergifyio backport master

@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2021

backport master

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

@kaizencc kaizencc marked this pull request as ready for review November 22, 2021 20:50
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f10452a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Nov 22, 2021
@mergify mergify bot merged commit 66e37cc into v2-main Nov 22, 2021
@mergify mergify bot deleted the kaizen-v2-main branch November 22, 2021 21:22
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 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 bot pushed a commit that referenced this pull request Nov 22, 2021
- Adds `rewriteImports` and helper functions `rewriteReadmeImports` and `rewriteMonoPackageImports` to `aws-cdk-migration` package.
- `individual-package-gen` consumes `rewriteReadmeImports` for alpha modules.
- `individual-package-gen` treats rosetta fixtures as source files and rewrites imports there as well.
- Adds `aws-cdk-migration` as a dependency to `ubergen`.
- Refactors `rewriteImports` in `ubergen` to use the API in `aws-cdk-migration`. Has the side affect of removing the string-replace function that was introduced in this [PR](#14255). This changes the readme import style from `import { blah as blah } from 'aws-cdk-lib';` to `import * as blah from 'aws-cdk-lib/blah';`.
- fixes error in `cfnspec` that generated invalid import names for L1 module readmes.
- fixes bug in `cdk-build` that only executed one `post` command.
- fixes `verify-imports` scripts typo introduced previously (that was not caught due to the `cdk-build` bug).

----

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

(cherry picked from commit 66e37cc)

# Conflicts:
#	packages/@aws-cdk/cfnspec/lib/library-creation.ts
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2021

backport master

✅ Backports have been created

mergify bot added a commit that referenced this pull request Nov 22, 2021
…17573) (#17635)

This is an automatic backport of pull request #17573 done by [Mergify](https://mergify.com).
Cherry-pick of 66e37cc has failed:
```
On branch mergify/bp/master/pr-17573
Your branch is up to date with 'origin/master'.

You are currently cherry-picking commit 66e37cc.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   packages/@aws-cdk/cfnspec/test/libary-creation.test.ts
	modified:   packages/aws-cdk-lib/.gitignore
	modified:   packages/aws-cdk-lib/package.json
	modified:   packages/aws-cdk-lib/scripts/verify-imports-resolve-same.ts
	deleted:    packages/aws-cdk-lib/scripts/verify-readme-import-rewrites.ts
	modified:   packages/aws-cdk-migration/bin/rewrite-imports-v2.ts
	modified:   packages/aws-cdk-migration/lib/rewrite.ts
	modified:   packages/aws-cdk-migration/test/rewrite.test.ts
	modified:   packages/monocdk/.gitignore
	modified:   tools/@aws-cdk/cdk-build-tools/bin/cdk-build.ts
	modified:   tools/@aws-cdk/individual-pkg-gen/transform-packages.ts
	modified:   tools/@aws-cdk/ubergen/bin/ubergen.ts
	modified:   tools/@aws-cdk/ubergen/package.json

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   packages/@aws-cdk/cfnspec/lib/library-creation.ts

```


To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the [documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ws#17573) (aws#17635)

This is an automatic backport of pull request aws#17573 done by [Mergify](https://mergify.com).
Cherry-pick of 66e37cc has failed:
```
On branch mergify/bp/master/pr-17573
Your branch is up to date with 'origin/master'.

You are currently cherry-picking commit 66e37cc.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   packages/@aws-cdk/cfnspec/test/libary-creation.test.ts
	modified:   packages/aws-cdk-lib/.gitignore
	modified:   packages/aws-cdk-lib/package.json
	modified:   packages/aws-cdk-lib/scripts/verify-imports-resolve-same.ts
	deleted:    packages/aws-cdk-lib/scripts/verify-readme-import-rewrites.ts
	modified:   packages/aws-cdk-migration/bin/rewrite-imports-v2.ts
	modified:   packages/aws-cdk-migration/lib/rewrite.ts
	modified:   packages/aws-cdk-migration/test/rewrite.test.ts
	modified:   packages/monocdk/.gitignore
	modified:   tools/@aws-cdk/cdk-build-tools/bin/cdk-build.ts
	modified:   tools/@aws-cdk/individual-pkg-gen/transform-packages.ts
	modified:   tools/@aws-cdk/ubergen/bin/ubergen.ts
	modified:   tools/@aws-cdk/ubergen/package.json

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   packages/@aws-cdk/cfnspec/lib/library-creation.ts

```


To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the [documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws-cdk-lib Related to the aws-cdk-lib package contribution/core This is a PR that came from AWS.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants