amplify-backend icon indicating copy to clipboard operation
amplify-backend copied to clipboard

fix info command unsupported warning

Open ykethan opened this issue 1 year ago • 13 comments

Problem

Issue number, if available: closes: https://github.com/aws-amplify/amplify-backend/issues/1000

when using node versions such as node 21, the cdk info command shows

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!                                                                                                                      !!
!!  This software has not been tested with node v21.6.1.                                                                !!
!!  Should you encounter odd runtime issues, please try using one of the supported release before filing a bug report.  !!
!!                                                                                                                      !!
!!  This software is currently running on node v21.6.1.                                                                 !!
!!  As of the current release of this software, supported node releases are:                                            !!
!!  - ^20.0.0 (Planned end-of-life: 2026-04-30)                                                                         !!
!!  - ^18.0.0 (Planned end-of-life: 2025-04-30)                                                                         !!
!!                                                                                                                      !!
!!  This warning can be silenced by setting the JSII_SILENCE_WARNING_UNTESTED_NODE_VERSION environment variable.        !!
!!                                                                                                                      !!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Changes

Corresponding docs PR, if applicable: add env variable to JSII_SILENCE_WARNING_UNTESTED_NODE_VERSION this execa process to silence the warning

tested locally with node 21 image

Validation

Checklist

  • [ ] If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • [ ] If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • [ ] If this PR requires a docs update, I have linked to that docs PR above.
  • [ ] If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

ykethan avatar Mar 01 '24 05:03 ykethan

🦋 Changeset detected

Latest commit: d770bf9261ee0f8491a85f841c8939d4e818ce57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/backend-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Mar 01 '24 05:03 changeset-bot[bot]

I 100% agree with @Amplifiyer . Hiding compatibility warnings is counter productive to debugabbility.

I'm fine with suppressing this warning, but we should replace it with our own warning if customer's node version is outside of amplify's supported versions.

If we do this we should also establish mechanism to track CDK supported Node, so that we proactively make changes on our side. Therefore my recommendation is to "do nothing" at this time.

sobolk avatar Mar 01 '24 18:03 sobolk

Instead of this, shouldn't we follow the CDK model and update our supported node versions. If CDK doesn't work with node 21, we won't be able to fix customer issues anyways.

I'm fine with suppressing this warning, but we should replace it with our own warning if customer's node version is outside of amplify's supported versions.

I was thinking if this can be a callout on the create flow/sandbox but a warning in a informational output/command doesn't feel like great DX tho

ykethan avatar Mar 01 '24 18:03 ykethan

I was thinking if this can be a callout on the create flow/sandbox but a warning in a informational output/command doesn't feel like great DX tho

Customers can always change their node version after the create-amplify flow, or use a different version of node in their CI/CD pipeline. This is an ideal DX for developers in my opinion to warn them when they are using unsupported runtime.

Amplifiyer avatar Mar 01 '24 18:03 Amplifiyer

Is this message suppressed anywhere else? It feels odd that it creeps up here, and while it's an important message and folks should be aware, it's clogging the output of what we care about which is the AWS environment variables. CDK rightfully obfuscates sensitive values which was one of the motivations for using CDK to read these values. imo it does not provide a ton of value in info output in its current place.

Would it be better if this was lifted to the beginning or end of the output instead of interweaved in the env info?

josefaidt avatar Mar 01 '24 21:03 josefaidt

It's also not entirely clear that this message is from CDK, not Amplify. Additionally, node version is already captured towards the top of the output.

josefaidt avatar Mar 01 '24 21:03 josefaidt

My 2c: suppress this warning in info output and add a warning that gets printed as the first line when running any amplify or create-amplify command with an unsupported node version

And then do we need our own environment variable like AMPLIFY_SUPPRESS_UNSUPPORTED_NODE_VERSION_WARNING...?

edwardfoyle avatar Mar 01 '24 22:03 edwardfoyle

My 2c: suppress this warning in info output and add a warning that gets printed as the first line when running any amplify or create-amplify command with an unsupported node version

agreed

And then do we need our own environment variable like AMPLIFY_SUPPRESS_UNSUPPORTED_NODE_VERSION_WARNING...?

oh goodness, could this be controlled by the log level?

josefaidt avatar Mar 01 '24 22:03 josefaidt

can we add an upper bound on the existing packages.json#engines key to align with CDK's requirements? https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/package.json#L23

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines

this should print the node version warning

josefaidt avatar Mar 02 '24 01:03 josefaidt

can we add an upper bound on the existing packages.json#engines key to align with CDK's requirements?

That would print a warning when installing, but would not print a warning after that. However, since we are suppressing installation output in create-amplify customers would not see it.

edwardfoyle avatar Mar 04 '24 18:03 edwardfoyle

that is a good callout. You'd only see it in the pipeline or on the next clone + setup. With the addition of amplify check, could we move it there (without the obnoxious !!!...)? Ideally that will be a command that is part of your PR checklist

and/or on sandbox startup? (which is essentially amplify check)

josefaidt avatar Mar 05 '24 00:03 josefaidt

Just confirmed that sandbox also doesn't show this warning along with create-amplify. @ykethan is creating github issues to look into surfacing runtime compatibility warnings for those two commands.

Since currently this seems to be the only place where this warning is surfacing, I'd say let's wait to have this (or our own) warning in the sandbox/create-amplify or pipeline-deploy commands before removing it from the info command.

Amplifiyer avatar Mar 06 '24 21:03 Amplifiyer

following up on this, on create-amplify and subsequent installations the engines key should be sufficient -- customers are warned by the package manager tool. https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/package.json#L22-L24

josefaidt avatar Mar 28 '24 18:03 josefaidt