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

Store Language Name, Version, and Requirements for npm, pnpm, and yarn #11017

Merged

Conversation

kbukum1
Copy link
Contributor

@kbukum1 kbukum1 commented Nov 26, 2024

What are you trying to accomplish?

This PR adds functionality to detect and include the language name (Node.js) and its version for npm, pnpm, and yarn package managers.

Why?

  • To accurately identify the runtime environment (Node.js) by using node -v to fetch the version directly.
  • This ensures dependency updates are handled with the correct language version context, improving reliability in JavaScript-based ecosystems.

This change improves Dependabot's ability to work with JavaScript projects by explicitly detecting and documenting Node.js as the runtime language and its version.


Anything you want to highlight for special attention from reviewers?

  • The implementation uses the node -v command to fetch the Node.js version at runtime. This approach is simple, reliable, and avoids relying on indirect metadata (e.g., volta or engines.node).
  • Test coverage ensures that all three package managers (npm, pnpm, and yarn) work seamlessly with this addition.

How will you know you've accomplished your goal?

  • Unit tests validate the language detection and version fetching logic.
  • Manually testing against sample npm, pnpm, and yarn projects confirms the correct Node.js version is detected.
  • Dependabot updates dependencies without issues when language-specific constraints are considered.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@kbukum1 kbukum1 changed the base branch from main to kamil/add_npm_yarn_pm_reqs_to_ecosystem_metrics November 26, 2024 04:02
@kbukum1 kbukum1 marked this pull request as ready for review November 26, 2024 04:02
@kbukum1 kbukum1 requested a review from a team as a code owner November 26, 2024 04:02
@kbukum1 kbukum1 marked this pull request as draft November 26, 2024 04:02
@kbukum1 kbukum1 marked this pull request as ready for review November 26, 2024 04:41
Base automatically changed from kamil/add_npm_yarn_pm_reqs_to_ecosystem_metrics to main November 26, 2024 06:21
@kbukum1 kbukum1 force-pushed the kamil/add_npm_yarn_language_and_reqs_to_ecosystem_metrics branch from 450537d to 27a7269 Compare November 26, 2024 06:28
Copy link
Member

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

it's a little hard to see the functional changes amid all of the added logging. In the future if you're adding logging to otherwise untouched code, can you bundle it as a separate change/PR?

Re: the new logging - much of the added Dependabot::Logger.info output looks like debug information which can get noisy if it's always written to logging. Would it make sense to use Dependabot::Logger.debug to keep noise down in prod logs but still make it easy to toggle on when debugging?

@kbukum1
Copy link
Contributor Author

kbukum1 commented Nov 26, 2024

it's a little hard to see the functional changes amid all of the added logging. In the future if you're adding logging to otherwise untouched code, can you bundle it as a separate change/PR?

Re: the new logging - much of the added Dependabot::Logger.info output looks like debug information which can get noisy if it's always written to logging. Would it make sense to use Dependabot::Logger.debug to keep noise down in prod logs but still make it easy to toggle on when debugging?

@jonabc
When monitoring metrics, it was challenging to identify which version was being used and what information was captured. This led me to believe that logging this general information properly at the info level would be helpful. It allows anyone investigating an issue to clearly see the package manager and language versions involved, potentially replicating the problem with the same versions. For this reason, I felt info was more appropriate than debug.

I also wanted to note that, in our investigations, errors related to run commands are often the hardest to diagnose because we lack proper logging for them. That’s why I’ve focused on adding logging, particularly around command execution.

Let me know if you think this could be approached differently!

@kbukum1 kbukum1 requested a review from jonabc November 26, 2024 20:29
Copy link
Member

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

I don't have enough practical experience with dependabot-core to know whether my previous comments are valid. I'm still a little uncomfortable with the change but 🤷 the functional changes seem small and tested :shipit:

@kbukum1 kbukum1 merged commit f7a116d into main Nov 26, 2024
146 checks passed
@kbukum1 kbukum1 deleted the kamil/add_npm_yarn_language_and_reqs_to_ecosystem_metrics branch November 26, 2024 22:04
@kbukum1 kbukum1 self-assigned this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants