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 explicit nodejs dependencies list #1384

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Conversation

mjeffryes
Copy link
Member

Cleaning up some likely copy-pasta:

While resolving a bug caused by explicit dependencies on old nodejs packages in pulumi-kubernetes (pulumi/pulumi-kubernetes#2857), we noticed that aws-native also has these dependencies, even though it does not need them because it does not implement any overlay resources.

@mjeffryes mjeffryes added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 2, 2024
@mjeffryes mjeffryes requested a review from a team March 2, 2024 00:02
Copy link
Contributor

github-actions bot commented Mar 2, 2024

Does the PR have any schema changes?

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

Copy link

codecov bot commented Mar 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 22.80%. Comparing base (09dbce2) to head (8938785).

Files Patch % Lines
provider/pkg/schema/gen.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
+ Coverage   22.71%   22.80%   +0.08%     
==========================================
  Files          25       25              
  Lines        4226     4210      -16     
==========================================
  Hits          960      960              
+ Misses       3105     3089      -16     
  Partials      161      161              

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

@mjeffryes mjeffryes self-assigned this Mar 2, 2024
@t0yv0
Copy link
Member

t0yv0 commented Mar 4, 2024

I'm a little confused about what this does.

We seem to have a good place for documenting this part of schema but the current documentation is laconic and leaves me wondering.
https://www.pulumi.com/docs/using-pulumi/pulumi-packages/schema/#language-specific-extensions

Do these settings inform the SDK generator for the particular language, and that only? How do they affect end-users as consumers of the said SDK?

In particular, won't removing these dependencies break the build of the Node SDK? Doesn't seem to, in the PR.

@mjeffryes
Copy link
Member Author

I'm a little confused about what this does.

We seem to have a good place for documenting this part of schema but the current documentation is laconic and leaves me wondering. https://www.pulumi.com/docs/using-pulumi/pulumi-packages/schema/#language-specific-extensions

Do these settings inform the SDK generator for the particular language, and that only? How do they affect end-users as consumers of the said SDK?

In particular, won't removing these dependencies break the build of the Node SDK? Doesn't seem to, in the PR.

As I understand it:

  • Yes, these settings are specific to the generator for each language.
  • The impact of this specific change is that we no longer add a bunch of explicit dependencies to the package.json for the node SDK (seen here in the diff: https://github.com/pulumi/pulumi-aws-native/pull/1384/files#diff-0d53ce0d334b84822a2dbdf673d0456efbacb59a76f72aabbfb1b7f8203a58f7)
  • Other providers, like Kubernetes, that add overlay resources to the SDK add explicit dependencies via this mechanism so the overlays in the SDK will compile.
  • AWS Native has never had any overlay resources, and thus is just fine with the default generated package.json for its Node SDK
  • It's very likely, these dependencies were just copy pasted from Kubernetes when this provider was created.

@r2el
Copy link

r2el commented Mar 4, 2024

We are running into an issue updating pulumi-aws-native in our package.json as result we are getting compilation errors. We believe this PR may fix our issue due to the old version of glob is bringing in an old version of minimatch that is producing the error for us.

node_modules/minimatch/dist/cjs/index"' has no exported member 'IOptions'.

26     interface IOptions extends minimatch.IOptions {

While we were searching Issues for this repo nothing seemed to come up, but when checking Pull Requests we noticed the removal of the old glob dependency in the files changed for this PR as a likely candidate.

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.

So we expect that removing this customization from nodejs enables the Node SDK generator to substitute some default dependencies, like "@pulumi/pulumi" that the generated SDK still depends on, but this change removes extraneous dependencies like "glob". This seems to make sense 🚢

@mjeffryes mjeffryes merged commit c335893 into master Mar 5, 2024
17 checks passed
@mjeffryes mjeffryes deleted the rm_unused_node_deps branch March 5, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants