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

fix info command unsupported warning #1093

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ykethan
Copy link
Contributor

@ykethan ykethan commented Mar 1, 2024

Problem

Issue number, if available:
closes: #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.

Copy link

changeset-bot bot commented Mar 1, 2024

🦋 Changeset detected

Latest commit: d770bf9

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

Co-authored-by: Edward Foyle <foyleef@amazon.com>
@ykethan ykethan marked this pull request as ready for review March 1, 2024 17:45
Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

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.

@sobolk
Copy link
Member

sobolk commented Mar 1, 2024

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.

@ykethan
Copy link
Contributor Author

ykethan commented Mar 1, 2024

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

@Amplifiyer
Copy link
Contributor

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.

@josefaidt
Copy link
Contributor

josefaidt commented Mar 1, 2024

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
Copy link
Contributor

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.

@edwardfoyle
Copy link
Member

edwardfoyle commented Mar 1, 2024

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

@josefaidt
Copy link
Contributor

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
Copy link
Contributor

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

@edwardfoyle
Copy link
Member

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.

@josefaidt
Copy link
Contributor

josefaidt commented Mar 5, 2024

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)

@Amplifiyer
Copy link
Contributor

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.

@josefaidt
Copy link
Contributor

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

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.

env info with node 16 or 21 shows banner
5 participants