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

[3.1] Remove ceiling on pyyaml version #923

Closed
wants to merge 1 commit into from

Conversation

neoaggelos
Copy link

Forward port from #916

Description

PyYAML 6.0.1 fixes a known issue in PyYAML 6.0, this ensures the LTS 3.1 python-libjuju includes the fix

QA Steps

<Commands / tests / steps to run to verify that the change works:>

pip install -e . pyyaml==6.0.1

Notes & Discussion

3.0 should probably also be updated with this change, but we are mainly interested in the LTS 3.1 branch

@jujubot
Copy link
Contributor

jujubot commented Jul 24, 2023

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial @juanmanuel-tirado

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Jul 24, 2023

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial @juanmanuel-tirado

@neoaggelos neoaggelos changed the title Remove ceiling on pyyaml version [3.1 Remove ceiling on pyyaml version Jul 24, 2023
@neoaggelos neoaggelos changed the title [3.1 Remove ceiling on pyyaml version [3.1] Remove ceiling on pyyaml version Jul 24, 2023
@cderici
Copy link
Contributor

cderici commented Jul 25, 2023

Thanks for opening a PR, @neoaggelos! Are you exclusively on the 3.1 branch? That branch is no longer supported (which is why this change is not ported onto here). If you're using libjuju with a 3.1 juju controller, then you can go ahead and use the master branch or the latest pylibjuju 3.2.0.1 stable release. Both of them should work with 3.1 without a problem👍

Unless you specifically need the 3.1 branch (in which case we should think about this a bit more because we didn't port a lot more changes happened in other branches), I'd like to close this PR, as I'd like to remove the 3.1 branch eventually. wdyt?

@neoaggelos
Copy link
Author

If python-libjuju<3.3 is something we can pin, I'm OK! which python libjuju version should we rely on going forward if we are targeting Juju 3.1 LTS?

@cderici
Copy link
Contributor

cderici commented Jul 25, 2023

If python-libjuju<3.3 is something we can pin, I'm OK!

Why do you want to pin python-libjuju<3.3 ?

which python libjuju version should we rely on going forward if we are targeting Juju 3.1 LTS?

As long as the Juju API doesn't break compatibility (might happen around Juju 4.0), then pylibjuju latest releases should continue to support 3.1. Therefore, if you're using a 3.x controller, all you need should be just pip install juju.

@neoaggelos
Copy link
Author

As long as the Juju API doesn't break compatibility (might happen around Juju 4.0), then pylibjuju latest releases should continue to support 3.1. Therefore, if you're using a 3.x controller, all you need should be just pip install juju.

OK then. Basically my end goal is to not have pip install juju. that way, when juju 4.0 comes around, old integration tests will not break and I won't have to manually go and then pin juju to <versionthatjustbroke. So I might as well do that now.

From your comment, sounds like that is juju<4?

@cderici
Copy link
Contributor

cderici commented Jul 25, 2023

and I won't have to manually go and then pin juju to <versionthatjustbroke

I don't see how that's different than what you're suggesting right now to pin pylibjuju.

I understand why you don't want your upstream dependencies to break your project, and that's perfectly reasonable. It is also reasonable that breaking changes are sometimes inevitable, which is why we have major.minor.patch versioning. TLDR here is that we should never put you in a position where you need to care about any version smaller than the major version (i.e. we should never make a breaking change in any version other than the major).

Now, currently the only time that you're gonna have an incompatibility break in pylibjuju is when juju and pylibjuju moves on to a breaking latest and you don't wanna do that upgrade yet (e.g. people are still using 2.9 and they want to stay there for a little longer). In that case, whenever we move onto a non-backwards compatible version, we create a branch so that the users can pin it there while the latest is moving on. That's why in pylibjuju we currently maintain 2.9 and master branches, so that people can just pin pylibjuju < 3 if they want to continue using the juju 2.9 controller.

If you're using a 3.x controller, pip install juju should be fine, until: 1) we move on to a breaking version like 4.0 AND 2) you want to continue using a 3.x instead of moving onto 4.x. At that time, there will be a 3.x branch that's supported so you can then use that branch. But as I understand this, in the end, the decision to not use the latest Juju controller is yours, therefore it is your responsibility to manually pin pylibjuju to keep compatibility in your project if you choose to stay there. If you choose to upgrade your Juju controller, then pip install juju will continue to work as normal in 4.x as well, in which case you won't need to do anything at all.

Hope this all makes senses and I'm not confusing you :)

@neoaggelos
Copy link
Author

Yes, we want to test with specific Juju versions, at the moment 3.1 which is LTS, hence pinning pylibjuju as well.

Thanks for the comment, makes total sense, there is no need for this PR 👍

@neoaggelos neoaggelos closed this Jul 25, 2023
@neoaggelos neoaggelos deleted the fix/pyyaml branch July 25, 2023 18:36
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