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(cli): change virtualenv directory to .venv to comply with python recommendation #10995

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

AnderEnder
Copy link
Contributor

Replace virtualenv directory .env to .venv for the python templates

Closes #9134


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

@gitpod-io
Copy link

gitpod-io bot commented Oct 20, 2020

@AnderEnder AnderEnder marked this pull request as ready for review October 20, 2020 21:58
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

@AnderEnder changes look good! have you run the yarn integ-init or npm run integ-init script to verify that it still succeeds? (it does not get executed as part of the PR build)

@@ -45,9 +45,11 @@ $ # List the available template types & languages
$ cdk init --list
Available templates:
* app: Template for a CDK Application
└─ cdk init app --language=[java|typescript]
└─ cdk init app --language=[csharp|fsharp|java|javascript|python|typescript]
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for updating this!

@AnderEnder
Copy link
Contributor Author

AnderEnder commented Oct 22, 2020

have you run the yarn integ-init or npm run integ-init script to verify that it still succeeds? (it does not get executed as part of the PR build)

Unfortunately, I was not able to run it even on master.

$ yarn integ-cli
yarn run v1.22.10
$ npm run integ-cli-regression && npm run integ-cli-no-regression

> aws-cdk@0.0.0 integ-cli-regression /Users/ander/projects/aws-cdk/packages/aws-cdk
> npm run integ-cli-regression-latest-release && npm run integ-cli-regression-latest-code


> aws-cdk@0.0.0 integ-cli-regression-latest-release /Users/ander/projects/aws-cdk/packages/aws-cdk
> test/integ/run-against-dist test/integ/test-cli-regression-against-latest-release.sh

/Users/ander/projects/aws-cdk/packages/aws-cdk does not seem to be a built CDK distribution (change directory or use DIST_ROOT)
/Users/ander/projects/aws-cdk/packages/aws-cdk/test/integ/run-against-dist.bash: line 11: TRAPS[@]: unbound variable

@shivlaks
Copy link
Contributor

have you run the yarn integ-init or npm run integ-init script to verify that it still succeeds? (it does not get executed as part of the PR build)

Unfortunately, I was not able to run it even on master.

$ yarn integ-cli
yarn run v1.22.10
$ npm run integ-cli-regression && npm run integ-cli-no-regression

> aws-cdk@0.0.0 integ-cli-regression /Users/ander/projects/aws-cdk/packages/aws-cdk
> npm run integ-cli-regression-latest-release && npm run integ-cli-regression-latest-code


> aws-cdk@0.0.0 integ-cli-regression-latest-release /Users/ander/projects/aws-cdk/packages/aws-cdk
> test/integ/run-against-dist test/integ/test-cli-regression-against-latest-release.sh

/Users/ander/projects/aws-cdk/packages/aws-cdk does not seem to be a built CDK distribution (change directory or use DIST_ROOT)
/Users/ander/projects/aws-cdk/packages/aws-cdk/test/integ/run-against-dist.bash: line 11: TRAPS[@]: unbound variable

@AnderEnder - i had just the yarn integ-init target in mind as it'll only test the init templates. does that also fail for you?

@AnderEnder
Copy link
Contributor Author

AnderEnder commented Oct 23, 2020

@AnderEnder - i had just the yarn integ-init target in mind as it'll only test the init templates. does that also fail for you?

@shivlaks, unfortunately, the result is similar:

$ yarn integ-init
yarn run v1.22.10
$ test/integ/run-against-dist test/integ/init/test-all.sh
/Users/ander/projects/aws-cdk/packages/aws-cdk does not seem to be a built CDK distribution (change directory or use DIST_ROOT)
/Users/ander/projects/aws-cdk/packages/aws-cdk/test/integ/run-against-dist.bash: line 11: TRAPS[@]: unbound variable
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@shivlaks
Copy link
Contributor

shivlaks commented Oct 23, 2020

@AnderEnder - i had just the yarn integ-init target in mind as it'll only test the init templates. does that also fail for you?

@shivlaks, unfortunately, the result is similar:

$ yarn integ-init
yarn run v1.22.10
$ test/integ/run-against-dist test/integ/init/test-all.sh
/Users/ander/projects/aws-cdk/packages/aws-cdk does not seem to be a built CDK distribution (change directory or use DIST_ROOT)
/Users/ander/projects/aws-cdk/packages/aws-cdk/test/integ/run-against-dist.bash: line 11: TRAPS[@]: unbound variable
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@AnderEnder thanks for giving it a shot, that is unfortunate. i'll try to pull this change down and see if I can run it. but as a side note, we probably need to give a little more love in this area so contributors can run them without so much friction...

edit: Oct 26 - will create an issue re: running integ tests locally for the cli - which seems broken

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2020

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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 6117f1c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2020

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 mergify bot merged commit a4a41b5 into aws:master Oct 27, 2020
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.

[cli] Create Python virtual environment in .venv directory
4 participants