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: silence node end of life warning in tests #4077

Merged
merged 5 commits into from
May 4, 2023
Merged

Conversation

vinayak-kukreja
Copy link
Contributor

@vinayak-kukreja vinayak-kukreja commented May 1, 2023

Currently tests are failing with end of life warning messages. This PR is adding env variable for tests to silence these errors.


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

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 1, 2023
@vinayak-kukreja vinayak-kukreja marked this pull request as ready for review May 1, 2023 22:28
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label May 2, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I'm not against this, but:

  • Someone made the decision to make this non-silencable to begin with. What was their reasoning, and shouldn't we check with them?
  • At this point, I don't see the value in having all these environment variables anymore. Can't we just have 1, JSII_SILENCE_NODE_WARNINGS for all of them? This is starting to get ridicilous.

@mrgrain
Copy link
Contributor

mrgrain commented May 2, 2023

See 880dbcc

Maybe @RomainMuller can comment on a preferred solution to this.

@vinayak-kukreja
Copy link
Contributor Author

Thank you both, I will check with Romain for this.

@rix0rrr
Copy link
Contributor

rix0rrr commented May 3, 2023

Also we can switch jobs to Node 16

@RomainMuller
Copy link
Contributor

I made the decision to make the EOL warnings un-silence-able. But I don't have an extremely strong opinion on this.

The general line of reasoning is that we reserve the right to pull support of EOL runtime releases a month after they went EOL, and people should stop using those as soon as possible (security risk, risking new bugs we won't necessarily be able to address, etc...).

In general, this is to make it easier for us to "move the dial" of minimum supported node release without having to stand by endlessly waiting for some notice to have been "seen" by enough folks... So we can start using new runtime features (e.g: in the past there'd have been a strong desire to use WeakRef and related APIs, but our minimum supported node did not have those, and it took AGES to move forward) and safety mechanisms (such as using the node: imports for node standard library, to avoid surprising resolutions & make it clear what is "built-in" versus "from dependencies").

I'm not against silencing this. I would however somewhat prefer if silencing the EOL warning required setting the environment variable to a specific value that implies users acknowledged that they should be moving to a non-EOL runtime as soon as possible...

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@vinayak-kukreja
Copy link
Contributor Author

The general line of reasoning is that we reserve the right to pull support of EOL runtime releases a month after they went EOL, and people should stop using those as soon as possible (security risk, risking new bugs we won't necessarily be able to address, etc...).

I see it has not yet been a month since EOL of Node 14 and we test with node 14 in our main workflow. I have not yet updated the github workflows with node 16, will make a separate PR for them but it should probably be merged after we complete the month timeline. WDYT?

@vinayak-kukreja
Copy link
Contributor Author

* At this point, I don't see the value in having all these environment variables anymore. Can't we just have 1, `JSII_SILENCE_NODE_WARNINGS` for all of them? This is starting to get ridicilous.

I see, but that would hide the specific error. For instance, looks like all of these are there for different reasons,

        "JSII_SILENCE_WARNING_KNOWN_BROKEN_NODE_VERSION",
        "JSII_SILENCE_WARNING_UNTESTED_NODE_VERSION",
        "JSII_SILENCE_WARNING_DEPRECATED_NODE_VERSION",
        "JSII_SILENCE_WARNING_END_OF_LIFE_NODE_VERSION",

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@rix0rrr
Copy link
Contributor

rix0rrr commented May 3, 2023

I see, but that would hide the specific error. For instance, looks like all of these are there for different reasons,

I understand that we can. I’m just asserting that we won’t.

And since we won’t, this feature is actually more of a hindrance than a help.

@rix0rrr
Copy link
Contributor

rix0rrr commented May 3, 2023

I won’t block the PR on this. But I’d like you to see my point that a theoretical feature that is never used in practice is a liability, not an asset

@@ -21,14 +21,18 @@ export function checkNode(envPrefix = 'JSII'): void {
'Should you encounter odd runtime issues, please try using one of the supported release before filing a bug report.';

if (nodeRelease?.endOfLife) {
const silenceVariable = `${envPrefix}_SILENCE_WARNING_END_OF_LIFE_NODE_VERSION`;
const acknowledgeNodeEol =
'I acknowledge end of life(EOL) status for this node version. The CDK reserves the right to pull support of EOL runtime releases a month after they went EOL status, and people are recommended to stop using those as soon as possible';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'I acknowledge end of life(EOL) status for this node version. The CDK reserves the right to pull support of EOL runtime releases a month after they went EOL status, and people are recommended to stop using those as soon as possible';
'Node14 is now end of life (EOL) as of April 30, 2023. Support of EOL runtimes are only guaranteed for 30 days after EOL. By silencing this warning you acknowledge that you are using an EOL version of Node and will upgrade to a supported version as soon as possible.';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, updated.

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
@vinayak-kukreja vinayak-kukreja removed the pr/do-not-merge This PR should not be merged at this time. label May 3, 2023
@mergify
Copy link
Contributor

mergify bot commented May 3, 2023

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 3, 2023
@mergify
Copy link
Contributor

mergify bot commented May 3, 2023

Merging (with squash)...

@mergify mergify bot merged commit cc6d294 into main May 4, 2023
29 checks passed
@mergify mergify bot deleted the vkukreja/test-fix branch May 4, 2023 06:07
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label May 4, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants