Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Mar 28, 2024

The channels repository is now a release candidate for 1.0.0, so no more breaking changes are expected. We can now widen the dependency to allow for the final 1.0.0 release.

The channels repository is now a release candidate for 1.0.0, so no more
breaking changes are expected. We can now widen the dependency to allow
for the final 1.0.0 release.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax requested a review from a team as a code owner March 28, 2024 14:00
@llucax llucax self-assigned this Mar 28, 2024
@llucax llucax requested a review from shsms March 28, 2024 14:00
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Mar 28, 2024
@llucax llucax added this to the v1.0.0-rc6 milestone Mar 28, 2024
@llucax
Copy link
Contributor Author

llucax commented Mar 28, 2024

Setting auto-merge.

@llucax llucax enabled auto-merge March 28, 2024 14:00
@llucax llucax added the type:tech-debt Improves the project without visible changes for users label Mar 28, 2024
@shsms
Copy link
Contributor

shsms commented Mar 28, 2024

Why not do it after 1.0 is released, then we don't have to have rc1 as a minimum requirement.

@llucax
Copy link
Contributor Author

llucax commented Mar 28, 2024

Why not do it after 1.0 is released, then we don't have to have rc1 as a minimum requirement.

I don't see an issue with having the rc as a minimum requirement, and this approach means we don't block downstream users from upgrading to 1.0.0 final or even newer releases. Please have in mind we have long dependency chains, so maybe having this with == is preventing even other libraries from upgrading.

In libraries I think we should specify dependencies as wide as possible to avoid blocking projects from using the version they need and end up in dependency hell, which already happened (for example with api-common).

@shsms
Copy link
Contributor

shsms commented Mar 28, 2024

and this approach means we don't block downstream users from upgrading to 1.0.0 final or even newer releases.

I don't get this argument. There's no channel 1.0 to upgrade to yet, so the SDK is not blocking anyone from upgrading to channel 1.0. As soon as channel is released, which could even be today, we can take in a PR that widens the requirement to 1.0 to 2.0, without the Rc.

I don't see the benefit in doing this first.

@llucax
Copy link
Contributor Author

llucax commented Mar 28, 2024

Not all project will upgrade to 1.0.0 final right away, so we'll force projects that are in 1.0.0rc1 to get a PR to be able to work with the SDK. Maybe it is true that for the SDK might not be that relevant because the current state of breaking rc's that force downstream project to use ==, so they will still need a PR to upgrade to the SDK, so they could upgrade the channels dependency then too.

I'm widening the dependency for all projects I see with == 1.0.0-rc1 though because it will simplify the upgrade process. And even when that is not the case, using == 1.0.0-rc1 will prevent end users from getting bug fixes once 1.0.0 and further patch releases are out, unless they create a new PR and a new release to bump to >= 1.0.0.

Besides that, in general, there is no reason why we shouldn't include all the range of dependencies that are supported in libraries, so I will reverse the question, why do you want to have == here? For me it makes no sense, if the SDK will work with rc1+, the dependency declaration should say that, always, even after channels 1.0.0 is released.

@llucax
Copy link
Contributor Author

llucax commented Mar 28, 2024

In case it is not clear, let's say peak shaving is pinned the current SDK version and channels 1.0.0-rc1, channels 1.0.1 is released with a critical security fix. Now dependabot creates a PR to upgrade, but the PR fails because it will also need a bump to the SDK (as the SDK is using == 1.0.0-rc1, so there is a dependency conflict), which means breaking changes and a huge amount of work, just to get a channel security fix that would have worked perfectly with the current SDK version.

@shsms
Copy link
Contributor

shsms commented Mar 28, 2024

Well, the SDK hasn't had a release since the upgrade to rc1, and nobody is using anything close to the latest release in production. I'd rather we go to 1.0.0 before people do start relying on rc1, so that what you say doesn't actually happen, and then it the sdk keeps having to say rc1, but if you must, then sure.

@llucax llucax added this pull request to the merge queue Mar 28, 2024
@llucax
Copy link
Contributor Author

llucax commented Mar 28, 2024

I prefer to keep the policy of having supported dependency ranges as wide as possible, because even if you might be right that with the current state of the SDK this scenario is very unlikely, I've been surprised by unexpected dependency conflicts too many times, so I see no reason why not go for the safe bet. And I still don't see any issues in keeping the RC as the minimum supported version if it is really the minimum version that works. Doing anything else is just introducing unnecessary potential dependency conflicts.

@shsms
Copy link
Contributor

shsms commented Mar 28, 2024

It is only solving the problems of imaginary users IMO, but you're right that it is not exactly an issue for the SDK.

Merged via the queue into frequenz-floss:v1.x.x with commit 2947dc5 Mar 28, 2024
@llucax llucax deleted the widen-channels branch March 28, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) type:tech-debt Improves the project without visible changes for users

Projects

Development

Successfully merging this pull request may close these issues.

2 participants