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.

Sign up for GitHub

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

Jump to bottom

Initial update to CDKv2 #1

Merged
merged 6 commits into from
Sep 16, 2022
Merged

Initial update to CDKv2 #1

merged 6 commits into from
Sep 16, 2022

Conversation

jgreenbow-userful
Copy link
Contributor

  • updated requirements files
  • replacement of aws_cdk.core with explicit imports
  • use of constructs module
  • removal of pylint useless option values

@jgreenbow-userful
Copy link
Contributor Author

Issue: does not pass tests due to incompatibilities between cdk-chalice (CDKv1 only) and chalice[cdkv2] implementation. Someone with experience in chalice could take it from here, or we could drop the test for now.

@jgreenbow-userful
Copy link
Contributor Author

jgreenbow-userful commented Jun 16, 2022 •
edited
Loading

EDIT: snipped error log to file

$ ./scripts/run-tests.sh

aws-cdk-project-structure-python_run-tests_01.log

@alexpulver
Copy link
Contributor

Hi @jgreenbow-userful, thanks for the work and submitting the pull request! Since this code supports an already published blog post, I sadly can't merge these changes, as it will require changes to the blog post itself. Mentioning just in case - cdk-chalice and chalice[cdk]/chalice[cdkv2] are two different implementations, and replace each other. For the blog post this code supports, it doesn't matter much which one to use, as it doesn't affect the recommendation on project structure. When I bumped pylint version in another project of mine, I also suddenly saw the first two options becoming a no-op, although they worked before. Please let me know if you have any thoughts on that, and again, thank you for submitting the pull request!

@alexpulver
Copy link
Contributor

P.S.: I am looking into supporting CDK v2 in cdk-chalice: alexpulver/cdk-chalice#93

@jgreenbow-userful
Copy link
Contributor Author

@alexpulver That all makes sense, and I should add that I appreciate your original recommendations and project structure.

That being said, this repo is quite a bit less useful if it needs significant modification to update before it can be used. How can users get access to a CDKv2 version? A new repo in aws-samples? A branch in this repo and remark in the blog post or README?

Just wondering what options exist to make this project useful to future CDKers. Thank you again, it has been a great launch point!

@jgreenbow-userful
Copy link
Contributor Author

I also suppose the answer may have to wait until cdk2-chalice gets merged.

Would you consider merging this pull request into a branch other than main? Or shall we close it?

@alexpulver alexpulver changed the base branch from main to chalice-cdkv2 June 17, 2022 19:52
@alexpulver
Copy link
Contributor

Thank you for the kind words 🤗. I liked your idea of a separate branch - created chalice-cdkv2 branch and made it to be the base branch for the pull request. Once I merge the pull request, I'll add a note about the new branch in the README.md file on the main branch. Can you please add the fact of migrating from cdk-chalice library to chalice[cdkv2] in the commit description?

@jgreenbow-userful
Copy link
Contributor Author

👍 Updated the commit description.

alexpulver
alexpulver requested changes Jun 19, 2022
View reviewed changes
Copy link
Contributor

@alexpulver alexpulver left a comment

Choose a reason for hiding this comment

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

Thanks! I have a few comments which are mostly cosmetic - can you please take a look?

app.py Outdated Show resolved Hide resolved
constants.py Outdated Show resolved Hide resolved
database/infrastructure.py Outdated Show resolved Hide resolved
deployment.py Outdated Show resolved Hide resolved
deployment.py Show resolved Hide resolved
requirements.in Outdated Show resolved Hide resolved
requirements.in Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
tests/test_api_infrastructure.py Outdated Show resolved Hide resolved
pipeline.py Outdated Show resolved Hide resolved
@jgreenbow-userful
Initial update to CDKv2
2f542d9 
Imports adjusted
- updated requirements files
- replacement of aws_cdk.core with aws_cdk
- import new constructs module

Cleared cdk context values

Updated aws-cdk CLI to compatible version
@jgreenbow-userful
Removed ignored pylint options
8197aca
@jgreenbow-userful
Replaced cdk-chalice with chalice[cdkv2]
08ed3a2 
As cdk-chalice has not been updated for cdkv2 yet, chalice[cdkv2]
provides an alternative implementation for these constructs.
However, this breaks the current tests.

Bumped pip-tools for requirements resolution
@jgreenbow-userful
Pinned flake8 to prevent dependency conflict
0b7d1f6 
flake8>=4.0 pins importlib-metadata<4.3 as a dependency. This is incompatible
with other packages requiring importlib-metadata>=4.12.
@jgreenbow-userful
Resolved pylint W1514: unspecified encoding
42d022a
@jgreenbow-userful
Updated token values in chalice config
eac039a
@alexpulver
Copy link
Contributor

alexpulver commented Aug 20, 2022 •
edited
Loading

Hi @jgreenbow-userful, thanks for submitting the changes! I am on vacation until September, hence didn't look into the review yet. I will go over that when I return.

@jgreenbow-userful
Copy link
Contributor Author

No worries @alexpulver , I have been enjoying some vacation time myself. This submission is not in any critical path, so no reason for urgency. Make the most of your time away!

alexpulver
alexpulver requested changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@alexpulver alexpulver left a comment

Choose a reason for hiding this comment

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

One last comment and we are done 😃

requirements-dev.in Show resolved Hide resolved
@alexpulver alexpulver merged commit d7b27b5 into aws-samples:chalice-cdkv2 Sep 16, 2022
@alexpulver
Copy link
Contributor

@jgreenbow-userful thanks a lot for your work on this! We further discussed internally, and want to update the blog post and the trunk branch implementation to AWS CDK v2 using your commit. I anyway wanted to update the blog post and this project with some changes to recommended project structure and internal implementation, so it is a great opportunity to do both together.

@jgreenbow-userful
Copy link
Contributor Author

@alexpulver Thanks so much, that's great to hear!

alexpulver pushed a commit that referenced this pull request Sep 22, 2022
@jgreenbow-userful @alexpulver
Initial update to CDKv2 (#1)
c49c112 
* Initial update to CDKv2

Imports adjusted
- updated requirements files
- replacement of aws_cdk.core with aws_cdk
- import new constructs module

Cleared cdk context values

Updated aws-cdk CLI to compatible version

* Removed ignored pylint options

* Replaced cdk-chalice with chalice[cdkv2]

As cdk-chalice has not been updated for cdkv2 yet, chalice[cdkv2]
provides an alternative implementation for these constructs.
However, this breaks the current tests.

Bumped pip-tools for requirements resolution

* Pinned flake8 to prevent dependency conflict

flake8>=4.0 pins importlib-metadata<4.3 as a dependency. This is incompatible
with other packages requiring importlib-metadata>=4.12.

* Resolved pylint W1514: unspecified encoding

* Updated token values in chalice config
@alexpulver
Copy link
Contributor

@jgreenbow-userful FYI I updated the mainline and the blog post with AWS CDK v2 + some additional changes to project structure and the component we wanted to do. Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexpulver alexpulver alexpulver approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jgreenbow-userful @alexpulver