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

Removing unwanted dev dependency #9838

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Removing unwanted dev dependency #9838

merged 3 commits into from
Apr 5, 2024

Conversation

niteshy
Copy link
Contributor

@niteshy niteshy commented Mar 31, 2024

resolves #9830

Avoid causing problem similar to #9830

Problem

protobuf is listed as dependencies in both setup.py & dev-requirements.txt. It cause problem as shown in #9830

Solution

Remove the duplicate entry from dev-requirements, as it is used in the prod code already.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

@niteshy niteshy requested a review from a team as a code owner March 31, 2024 17:38
@cla-bot cla-bot bot added the cla:yes label Mar 31, 2024
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@github-actions github-actions bot added the community This PR is from a community member label Mar 31, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.12%. Comparing base (71f3519) to head (7a2cdb8).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9838      +/-   ##
==========================================
- Coverage   88.12%   88.12%   -0.01%     
==========================================
  Files         178      178              
  Lines       22458    22449       -9     
==========================================
- Hits        19792    19783       -9     
  Misses       2666     2666              
Flag Coverage Δ
integration 85.57% <ø> (-0.01%) ⬇️
unit 61.89% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution 🙂 This will 100% reduce accidental dev/released version discrepancies that we want to avoid. One small nit in the change log to make things a bit clearer, but then we're good to go 👍

@@ -0,0 +1,6 @@
kind: Dependencies
body: Remove unwanted dependency of protobuf in dev-requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit:
Can we replace the word unwanted with duplicate here. The word unwanted without the context that this dependency is getting pulled in somewhere else makes it sound like we're dropping the dependency completely.

@QMalcolm
Copy link
Contributor

QMalcolm commented Apr 5, 2024

Gonna get this merged!

Of note, we don't need to backport this. We try to only do backports for bugs and security issues. The risk we incur by not backporting this in exceedingly low. The risk is that in the future we backport a change to the version specification of protobuf (which we've only ever had to do once) and only only update the dep in dev-requirements.txt (updating it in just setup.py or both setup.py and dev-requirements.txt isn't problematic). Given that we backport by cherry-picking, and now the main branch will no longer have the dependency in dev-requirements.txt, any backported version specification change to protobuf will include a change to setup.py.

@QMalcolm QMalcolm merged commit 96f5426 into dbt-labs:main Apr 5, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Duplicate dependency specifications means prod and dev version of protobuf might differ
3 participants