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

(amplify-alpha): customResponseHeaders does not support monorepo structures #31758

Open
1 task
georeeve opened this issue Oct 15, 2024 · 7 comments · May be fixed by #31771
Open
1 task

(amplify-alpha): customResponseHeaders does not support monorepo structures #31758

georeeve opened this issue Oct 15, 2024 · 7 comments · May be fixed by #31771
Labels
@aws-cdk/aws-amplify Related to AWS Amplify bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@georeeve
Copy link

Describe the bug

Firstly, I can't tell if this is a CDK or Amplify issue. aws-amplify/amplify-hosting#1995 seems related, but someone in that issue was using customHttp.yml which we can't do on CDK.

I've tried to condense this into a simple example app below. It works without the customResponseHeaders property. However, when I add customResponseHeaders, it still deploys via CloudFormation, but when the Amplify build runs I get the errors below.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The app deploys successfully on Amplify.

Current Behavior

The app errors on Amplify deploy:

!!!Monorepo spec provided without "applications" key
!!!Unable to save headers: CustomerError: Monorepo spec provided without "applications" key

Reproduction Steps

Example Amplify app:

const amplifyApp = new AmplifyApp(this, `AmplifyApp`, {
  buildSpec: BuildSpec.fromObjectToYaml({
    version: "1.0",
    applications: [
      {
        appRoot: "frontend",
        frontend: {
          phases: {
            preBuild: {
              commands: [
                "npm install -g pnpm",
                "pnpm install --frozen-lockfile",
              ],
            },
            build: {
              commands: ["pnpm run build"],
            },
          },
          artifacts: {
            baseDirectory: ".next",
            files: ["**/*"],
          },
          cache: {
            paths: [".next/cache/**/*"],
          },
        },
      },
    ],
  }),
  customResponseHeaders: [
    {
      pattern: "**",
      headers: {
        Test: "test",
      },
    },
  ],
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.162.1 (build 10aa526)

Framework Version

2.162.1-alpha.0

Node.js Version

v22.9.0

OS

macOS 14.7 (23H124)

Language

TypeScript

Language Version

TypeScript 5.6.3

Other information

No response

@georeeve georeeve added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 15, 2024
@github-actions github-actions bot added the @aws-cdk/aws-amplify Related to AWS Amplify label Oct 15, 2024
@georeeve
Copy link
Author

So it seems like CDK is creating this custom headers yaml, which is fine for regular applications:

customHeaders:
  - pattern: "**"
    headers:
      - key: Test
        value: test

However, for monorepos, this custom headers yaml is required:

applications:
  - appRoot: frontend
    customHeaders:
      - pattern: "**"
        headers:
          - key: Test
            value: test

When I change the yaml in the console, this passes the Amplify deployment. So should we add an optional appRoot property to CDK to generate the monorepo yaml? Something like:

customResponseHeaders: [
    {
        appRoot: 'frontend',
        pattern: '**',
        headers: {
            'Test': 'test',
        },
    },
],

I'm happy to try and submit a PR if so.

@ashishdhingra
Copy link
Contributor

ashishdhingra commented Oct 15, 2024

@georeeve Could you please share the following:

  • List of all packages used in the project. Package "@aws-cdk/aws-amplify-alpha": "2.162.1-alpha.0" has the construct named App, but there is no construct named AmplifyApp.
  • Output of cdk synth.

Thanks,
Ashish

@ashishdhingra ashishdhingra self-assigned this Oct 15, 2024
@ashishdhingra ashishdhingra added p2 needs-reproduction This issue needs reproduction. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 15, 2024
@georeeve
Copy link
Author

@ashishdhingra My apologies, I do import { App as AmplifyApp } from '@aws-cdk/aws-amplify-alpha'; in my code due to import naming conflicts with other packages I use.

I've tested this further and my first reply comment sums up what is actually happening. Currently the amplify package does not support adding custom response headers for monorepo structures due to the differences in yaml I outlined above. I've actually made a few changes to the amplify package locally and gotten it working for monorepos, I should be able to get a PR submitted tomorrow if you're willing to take a look?

@georeeve
Copy link
Author

Submitted as a draft now: #31771

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 16, 2024
@ashishdhingra
Copy link
Contributor

@georeeve Thanks for your response. Per CDK Amplify API doc, it does support Adding custom response headers. Please advise if you have tried using the customResponseHeaders property instead of specifying these in buildSpec.

The PR would be reviewed by CDK team on incoming priority.

Thanks,
Ashish

@ashishdhingra ashishdhingra added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 16, 2024
@georeeve
Copy link
Author

georeeve commented Oct 16, 2024

@ashishdhingra I'm a bit confused, I was specifying it in the customResponseHeaders above.

For clarity, Amplify supports two main repository structures. A single repository structure (non-monorepo) and a monorepo structure (see https://docs.amplify.aws/react/deploy-and-host/fullstack-branching/monorepos/). CDK does support setting custom response headers through the customResponseHeaders property, but currently only for non-monorepo structures, as in the background CDK just converts this to the first example code block in this YAML spec: https://docs.aws.amazon.com/amplify/latest/userguide/custom-header-YAML-format.html

My PR additionally adds support for monorepo structures, by converting it to the second example code block above. Given that this limitation is not explicitly stated in the docs, I'm guessing it's an oversight of the initial implementation. @ayush987goyal are you able to provide any more info? Looking through previous PRs, you added the property in #17102.

As an aside, I think you can specify it in the buildSpec instead, but it seems like that's deprecated: https://docs.aws.amazon.com/amplify/latest/userguide/migrate-custom-headers.html

@georeeve georeeve changed the title (amplify-alpha): Monorepo error when setting custom response headers (amplify-alpha): customResponseHeaders does not support monorepo structures Oct 16, 2024
@ashishdhingra
Copy link
Contributor

@georeeve Thanks for clarification and apologies for confusion.

@ashishdhingra ashishdhingra added effort/medium Medium work item – several days of effort and removed needs-reproduction This issue needs reproduction. labels Oct 16, 2024
@ashishdhingra ashishdhingra removed their assignment Oct 16, 2024
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-amplify Related to AWS Amplify bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants