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

chore: experimental cdk migrate command #25859

Merged
merged 31 commits into from
Jun 23, 2023
Merged

chore: experimental cdk migrate command #25859

merged 31 commits into from
Jun 23, 2023

Conversation

HBobertz
Copy link
Contributor

@HBobertz HBobertz commented Jun 5, 2023

cdk migrate command.

See readme for additional description of command. Default input path is a file named "template.txt" in the parent directory for now. Default output path is the current directory. If you have any issues with the actually generated code, please create an issue on https://github.com/iph/noctilucent

Please refer to noctilucent or command help supported languages to see what languages are supported.


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

@HBobertz HBobertz added the pr/do-not-merge This PR should not be merged at this time. label Jun 5, 2023
@gitpod-io
Copy link

gitpod-io bot commented Jun 5, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team June 5, 2023 21:36
@github-actions github-actions bot added the p2 label Jun 5, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 5, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jun 5, 2023
@HBobertz HBobertz marked this pull request as ready for review June 12, 2023 15:16
@HBobertz HBobertz removed the pr/do-not-merge This PR should not be merged at this time. label Jun 12, 2023
Copy link
Contributor

@Naumel Naumel left a comment

Choose a reason for hiding this comment

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

I have a couple of questions, if I just got the wrong understanding, we can clear it up and approve the changes.

Comment on lines 2233 to 2254
** noctilucent@0.2.0 - https://www.npmjs.com/package/noctilucent/v/0.2.0 | MIT
MIT License

Copyright (c) 2021 Sean Myers

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems standard. Do we have to involve anyone else to review the wording?

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'm honestly not sure, romain is the one who included this. I'm not even sure it's totally necessary since this gets auto genned anyway

Comment on lines 23 to 29
git -C ./vendor/noctilucent git fetch && git checkout 6da7c9fade55f8443bba7b8fdfcd4ebfe5208fb1
fi

# update the package to the pinned commit hash
git -C vendor/noctilucent reset --hard HEAD
git -C vendor/noctilucent fetch && git -C vendor/noctilucent checkout 6da7c9fade55f8443bba7b8fdfcd4ebfe5208fb1
Copy link
Contributor

Choose a reason for hiding this comment

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

You only need one line in the if, the other two get executed no matter what, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp ill fix this

export const MIGRATE_SUPPORTED_LANGUAGES: readonly string[] = nocti.supported_languages();

export async function cliMigrate(
inputpath: string = process.cwd() + '/../temp.txt',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the chance of (eventual) name collision?

This hardcodes a bunch of input that should be accepted from the command line. Will this actually output things in "~/my-dir" if the cli comand was cdk migrate -l java -inputpath ~/template.json -outputpath ~/my-dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup it works, these are just setting the default. We could just delete the /../temp.txt default for the input path if we wanted, I just left it in there for now

packages/aws-cdk/lib/commands/migrate.ts Outdated Show resolved Hide resolved
outputpath = process.cwd(),
) {
warning('This is a very experimental feature, use at your own risk.');
const type = 'default'; // "default" is the default type (and maps to 'app')
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to double-take: why is this an option here, but not exposed as an argument?
Even if right now it has a single value, it can be expanded in the future (based on the comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so this is basically just to get the init funcitonality to work. I'm happy to just pass the hardcoded string into the initliazeproject but I thought I'd be explicit about wat the default meant.

@RomainMuller RomainMuller force-pushed the experimental/migrate branch 2 times, most recently from f900fbf to afbe3b3 Compare June 23, 2023 11:45
RomainMuller and others added 3 commits June 23, 2023 14:33
This is an attempt to make sure that there is one and only one `ts-node`
in the closure, so that symlinks "always make sense".
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@HBobertz HBobertz added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Jun 23, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 23, 2023 15:40

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jun 23, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 3f1f974 into main Jun 23, 2023
8 of 9 checks passed
@mergify mergify bot deleted the experimental/migrate branch June 23, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants