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

Unify development dependencies #5974

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

Just curious to see what breaks if we unify our development dependencies.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Looks like nothing breaks. 😀

updater/Gemfile Show resolved Hide resolved
@deivid-rodriguez
Copy link
Contributor Author

The downside of this PR is that some dependencies are no longer locked in CI now. That means CI is less deterministic and potentially could get affected by issues in dependencies. For example, I guess there was a reason why vcr was pinned to 6.1.0. Now it's more unlocked.

But this may be acceptable.

@deivid-rodriguez
Copy link
Contributor Author

After a second look, the only dependency that got relaxed with these changes is vcr. I'm happy to change this to keep that pin.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/unify-dev-deps-with-updater branch from 6baf9e2 to 5384a2e Compare November 7, 2022 10:54
@deivid-rodriguez
Copy link
Contributor Author

I was not too happy about the previous approach, because it was adding dependencies to the updater that it didn't really need. I changed the approach so that the set of dependencies used by /updater is exactly the same.

It's probably a bit too hacky, but essentially the same thing other gemspecs are doing by pulling development dependencies from common.

Let me know what you think!

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

This way of doing it makes sense to me, totally fine with the hack, since these two libs/apps are effectively joined at the hip into a monorepo.

updater/Gemfile Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/unify-dev-deps-with-updater branch 2 times, most recently from 79f4c93 to b7e5587 Compare January 20, 2023 11:11
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review January 20, 2023 11:12
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner January 20, 2023 11:12
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/unify-dev-deps-with-updater branch from b7e5587 to 8d9aede Compare January 20, 2023 18:18
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

I think this is an okay approach. I agree with @jeffwidman that this is effectively a monorepo of core and updater anyway, so it makes sense to share dev dependencies.

I had one minor nit but it's not blocking

updater/Gemfile Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/unify-dev-deps-with-updater branch from 8d9aede to 459c6ee Compare January 20, 2023 18:35
@deivid-rodriguez deivid-rodriguez merged commit 8ea8936 into main Jan 20, 2023
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/unify-dev-deps-with-updater branch January 20, 2023 20:26
@deivid-rodriguez
Copy link
Contributor Author

It worked! 🥳

alcere pushed a commit that referenced this pull request Feb 20, 2023
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.

3 participants