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

Remove unused npm dependencies #4971

Merged
merged 1 commit into from
Dec 24, 2024
Merged

Remove unused npm dependencies #4971

merged 1 commit into from
Dec 24, 2024

Conversation

justinvp
Copy link
Member

It looks like the resolve and builtin-modules dependencies aren't actually used anywhere, so stop depending on them.

The motivation for this change is that the dependency on resolve causes errors to be logged in logs during plugin discovery, because the package includes tests that have malformed package.json files. If we don't actually need the dependency it'd be nice to remove it so those errors are no longer logged (so users don't ask us about them).

Also remove the builtin-modules dependency while cleaning this up, since it doesn't appear to be used either AFAICT.

Similar to #3238
Reference: pulumi/pulumi#17578

Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.42%. Comparing base (5037c71) to head (735e0e8).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4971   +/-   ##
=======================================
  Coverage   24.42%   24.42%           
=======================================
  Files         360      360           
  Lines      143403   143403           
=======================================
  Hits        35023    35023           
  Misses     108281   108281           
  Partials       99       99           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

These dependencies are from 2018 it seems reasonable that we stopped actually depending on these.

@justinvp
Copy link
Member Author

What's going on with the failing waf test? Something we need to clean up in a test account?

=== NAME  TestAccWAFRuleGroup_basic
    rule_group_test.go:34: Step 1/2 error: Error running apply: exit status 1
        
        Error: creating WAF Rule Group (tfaccze0ep): operation error WAF: CreateRuleGroup, https response error StatusCode: 400, RequestID: ac6aaf1f-be43-45b9-8044-589560f4e5b7, WAFLimitsExceededException: Operation would result in exceeding resource limits.
        
          with aws_waf_rule_group.test,
          on terraform_plugin_test.tf line 17, in resource "aws_waf_rule_group" "test":
          17: resource "aws_waf_rule_group" "test" {
        
--- FAIL: TestAccWAFRuleGroup_basic (15.67s)

justinvp added a commit to pulumi/pulumi-gitlab that referenced this pull request Dec 23, 2024
It looks like these dependencies aren't actually used anywhere, so stop
depending on them.

The motivation for this change is that the dependency on `resolve`
causes errors to be logged in logs during plugin discovery, because the
package includes tests that have malformed `package.json` files. If we
don't actually need the dependency it'd be nice to remove it so those
errors are no longer logged (so users don't [ask us about
them](pulumi/pulumi#17578)).

Also remove `read-package-tree` since it is deprecated.

Also remove `builtin-modules`, since it doesn't appear to be used
either.

Similar to pulumi/pulumi-aws#3238 and
pulumi/pulumi-aws#4971
Reference: pulumi/pulumi#17578
justinvp added a commit to pulumi/pulumi-digitalocean that referenced this pull request Dec 23, 2024
It looks like these dependencies aren't actually used anywhere, so stop
depending on them.

The motivation for this change is that the dependency on `resolve`
causes errors to be logged in logs during plugin discovery, because the
package includes tests that have malformed `package.json` files. If we
don't actually need the dependency it'd be nice to remove it so those
errors are no longer logged (so users don't [ask us about
them](pulumi/pulumi#17578)).

Also remove `read-package-tree` since it is deprecated.

Also remove `builtin-modules` since it doesn't appear to be used either.

Similar to pulumi/pulumi-aws#3238,
pulumi/pulumi-aws#4971, and
pulumi/pulumi-gitlab#786
Reference: pulumi/pulumi#17578
justinvp added a commit to pulumi/pulumi-vsphere that referenced this pull request Dec 23, 2024
It looks like these dependencies aren't actually used anywhere, so stop
depending on them.

The motivation for this change is that the dependency on `resolve`
causes errors to be logged in logs during plugin discovery, because the
package includes tests that have malformed `package.json` files. If we
don't actually need the dependency it'd be nice to remove it so those
errors are no longer logged (so users don't [ask us about
them](pulumi/pulumi#17578)).

Also remove `read-package-tree` since it is deprecated.

Also remove `builtin-modules` since it doesn't appear to be used either.

Similar to pulumi/pulumi-aws#3238,
pulumi/pulumi-aws#4971,
pulumi/pulumi-gitlab#786, and
pulumi/pulumi-digitalocean#914
Reference: pulumi/pulumi#17578
@t0yv0
Copy link
Member

t0yv0 commented Dec 23, 2024

Yeah I think so. Having a quick look.

@t0yv0
Copy link
Member

t0yv0 commented Dec 23, 2024

=== NAME  TestAccWAFRuleGroup_changeNameForceNew
    rule_group_test.go:75: Step 1/3 error: Error running apply: exit status 1

        Error: creating WAF Rule Group (tfaccyzotq): operation error WAF: CreateRuleGroup, https response error StatusCode: 400, RequestID: fcb7db05-54ed-4ab1-8a3b-cecdb02574fb, WAFLimitsExceededException: Operation would result in exceeding resource limits.

          with aws_waf_rule_group.test,
          on terraform_plugin_test.tf line 17, in resource "aws_waf_rule_group" "test":
          17: resource "aws_waf_rule_group" "test" {

ForceNew may be trying to create two of these. Scrolling back a little I think our intent with this test was to smoke-test upstream post-patching with their own tests, somehow we selected waf instead of the newer wafv2 module for this purpose. Rather than raising our account waf quotas we can disable this test or switch to wafv2.

@t0yv0
Copy link
Member

t0yv0 commented Dec 23, 2024

#5001 I'll land that first and rebase this to get it in.

It looks like `resolve` and `builtin-modules` dependencies aren't actually used anywhere, so stop depending on them.

The motivation for this change is that the dependency on `resolve` causes errors to be logged in logs during plugin discovery, because the package includes its tests that have malformed `package.json` files in its package. If we don't actually need the dependency, it'd be nice to remove it so those errors are no longer logged (so users don't have to ask us about them).

Also remove the `builtin-modules` dependency while cleaning this up, since it doesn't appear to be used either AFAICT.
@justinvp justinvp enabled auto-merge (squash) December 23, 2024 22:41
@justinvp justinvp merged commit c8c0044 into master Dec 24, 2024
25 checks passed
@justinvp justinvp deleted the justin/resolve branch December 24, 2024 00:52
@t0yv0
Copy link
Member

t0yv0 commented Dec 27, 2024

Thank you for getting this in!

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.

2 participants