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(appsync): removes codefirst schema generation #23250

Merged
merged 8 commits into from
Dec 7, 2022

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Dec 6, 2022

Removes all of the code first schema generation code in favor of this living in a separate module. This will make stabilizing the other constructs easier while allowing for continuing development of high level utilities within the aws-cdk-appsync-utilities library.

BREAKING CHANGE: Renames Schema to SchemaFile that implements ISchema. Removes all addXxx type methods from GraphQlApi.


All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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 Dec 6, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team December 6, 2022 19:35
@github-actions github-actions bot added the p2 label Dec 6, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 6, 2022
Removes all of the code first schema generation code in favor of this
living in a separate module. This will make stabilizing the other
constructs easier while allowing for continuing development of high
level utilities within the `aws-cdk-appsync-utilities` library.

BREAKING CHANGE: Renames `Schema` to `SchemaFile` that implements
`ISchema`. Removes all 'addXxx' type methods from `GraphQlApi`.
@MrArnoldPalmer MrArnoldPalmer force-pushed the chore/appsync-remove-codefirst branch from 95cf762 to 16c4a04 Compare December 6, 2022 19:40
@corymhall corymhall added the pr/do-not-merge This PR should not be merged at this time. label Dec 7, 2022
Copy link
Contributor

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

Looks good! Just one minor suggestion.

packages/@aws-cdk/aws-appsync/lib/schema.ts Outdated Show resolved Hide resolved
@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label Dec 7, 2022
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f351d5d
  • 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 Dec 7, 2022

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 2bd1e41 into main Dec 7, 2022
@mergify mergify bot deleted the chore/appsync-remove-codefirst branch December 7, 2022 23:25
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Dec 9, 2022
Removes all of the code first schema generation code in favor of this living in a separate module. This will make stabilizing the other constructs easier while allowing for continuing development of high level utilities within the `aws-cdk-appsync-utilities` library.

BREAKING CHANGE: Renames `Schema` to `SchemaFile` that implements `ISchema`. Removes all `addXxx` type methods from `GraphQlApi`.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

pfried commented Dec 29, 2022

Hey,

I would suggest putting at least a reference in the appsync docs that the code first approach has moved to another library or list it in the docs.

From the release notes you have to crawl through the commits and issues to find out what happened.

@MrArnoldPalmer
Copy link
Contributor Author

MrArnoldPalmer commented Dec 29, 2022

@pfried Added a reference here. Do you think it needs more visibility?

@pfried
Copy link

pfried commented Dec 30, 2022

@MrArnoldPalmer I think this is fine.

brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
Removes all of the code first schema generation code in favor of this living in a separate module. This will make stabilizing the other constructs easier while allowing for continuing development of high level utilities within the `aws-cdk-appsync-utilities` library.

BREAKING CHANGE: Renames `Schema` to `SchemaFile` that implements `ISchema`. Removes all `addXxx` type methods from `GraphQlApi`.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
Removes all of the code first schema generation code in favor of this living in a separate module. This will make stabilizing the other constructs easier while allowing for continuing development of high level utilities within the `aws-cdk-appsync-utilities` library.

BREAKING CHANGE: Renames `Schema` to `SchemaFile` that implements `ISchema`. Removes all `addXxx` type methods from `GraphQlApi`.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

gabsn commented Apr 21, 2023

Wow I'm super shocked i've developed my whole infra using cdk code-first approach and I'm just learning that this is not supported anymore. This is a really bold move... Also this makes me realize that current cdk pms have probably no experience in building large-scale application, otherwise they'd understand the benefit of the code-first approach. Available to help you in a user interview if you need more understanding of my use case.

@MrArnoldPalmer
Copy link
Contributor Author

@gabsn it's not that this isn't supported anymore, it's that we wanted to stabilize the appsync constructs but weren't sure if the existing code first implementation is ready for stabilization. We still own and vend CodeFirstSchema in awscdk-appsync-utils.

We (myself in particular) are very convinced that code first schema approach has a lot of advantages especially for large scale applications. I would like to spend more time developing the code first approach and explore even more functionality like type safe handler generation etc. There is a lot of exploration and discovery to be done in this area and if you're interested in helping at all that would definitely be appreciated.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants