Skip to content

Conversation

@gagik
Copy link
Contributor

@gagik gagik commented Dec 3, 2025

I cannnot reproduce this one locally but this seems to be in same spirit:
https://github.com/mongodb-js/mongosh/actions/runs/19894601536/job/57021652772#step:6:665

Maybe we should add webpack-build as part of check? I don't really understand how tests pass.

@gagik gagik requested a review from a team as a code owner December 3, 2025 14:31
Copilot AI review requested due to automatic review settings December 3, 2025 14:31
@gagik gagik added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Dec 3, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an auxiliary publishing issue related to @aws-sdk/client-sts in the webpack configuration. The change ensures that the optional peer dependency is properly externalized during the build process.

Key Changes:

  • Added @aws-sdk/client-sts to webpack externals configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

'os-dns-native': 'commonjs2 os-dns-native',
'system-ca': 'commonjs2 system-ca',
// @aws-sdk/client-sts is an optional peer dependency of @aws-sdk/credential-providers
'@aws-sdk/client-sts': 'commonjs2 @aws-sdk/client-sts',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm ... maybe a TODO ticket to clean this up?

But I don't think we should just make this a webpack external because it doesn't happen to get installed in CI. For Evergreen, we have a script (scripts/mark-ci-required-optional-dependencies.ts) to actually mark the "required optional" dependencies as required because we know we want them in CI (just not necessarily on user machines when they run e.g. npx mongosh). Maybe a similar approach makes sense here?

As part of the Node.js 24 upgrade project, we'll need to take care of making the @aws-sdk/credential-providers package a hard dependency of service-provider-node-driver, so this may change as part of that (I don't quite know how), hence the suggestion of a TODO ticket rather than doing something right now

@gagik gagik merged commit d206981 into main Dec 3, 2025
18 of 19 checks passed
@gagik gagik deleted the gagik/fix-publishing-2 branch December 3, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants