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

Fix deprecated code #677

Conversation

m-czernek
Copy link
Contributor

@m-czernek m-czernek commented Sep 2, 2024

What does this PR do?

This PR backports saltstack/salt@67b08ab

This PR fixes runtime errors connected to warn_until_date code that's been deprecated upstream. This PR works around the code removal in saltstack/salt@67b08ab

What issues does this PR fix or reference?

Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1226118

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@m-czernek m-czernek force-pushed the backport-django-returner-removal branch from 6f8a318 to dfc2087 Compare September 2, 2024 13:20
Copy link
Member

@agraul agraul left a comment

Choose a reason for hiding this comment

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

We don't use upstream's deprecation/removal policy and can't remove features in a maintenance update

@m-czernek
Copy link
Contributor Author

m-czernek commented Sep 2, 2024

@agraul I get that but the code doesn't work right now - we're including it, but if a user touches the code, it'll blow up with an exception. Isn't it the same state as right now? Would you prefer that we diverge from upstream and modify the dates?

@agraul
Copy link
Member

agraul commented Sep 3, 2024

From a quick look, I think we don't know if it's broken. Salt's deprecation mechanism warn_until_date raises the RuntimeError. That's a self-inflected wound, we could also move the date further out to the future. Then we don't get the RuntimeError anymore.

Salt uses this to force their developers to stick to the deprecation timeline and actually remove code once it reaches that date. For us it's annoying, this isn't the first time it has happened, because we have longer support cycles and don't want to drop features without moving to a new major version.

Due to SUSE's extended support policy, we won't remove
code from Salt until next major release.
@m-czernek m-czernek force-pushed the backport-django-returner-removal branch from dfc2087 to 6ac62f8 Compare September 4, 2024 08:00
@m-czernek
Copy link
Contributor Author

m-czernek commented Sep 4, 2024

@agraul I take it from your response that you would prefer for us to diverge from upstream, even if the code was blowing up until now (and therefore, should be safe to remove since we had no complaints).

In such case, this should be good enough. I set the date to 2026 but we should upgrade to 3007.x before that. LMK if you'd prefer to set a different date.

Question: shall I also adjust https://github.com/openSUSE/salt/blob/openSUSE/release/3006.0/salt/modules/aptpkg.py#L3130 ? We adjusted the date to 2025, but I'm quite sure we won't update to 3007.x by Jan 1st, 2025, right?

@agraul
Copy link
Member

agraul commented Sep 4, 2024

@agraul I take it from your response that you would prefer for us to diverge from upstream, even if the code was blowing up until now (and therefore, should be safe to remove since we had no complaints).

Yes. When it comes to the support lifecycle, we are not following upstream. Again, we can't drop features in minor updates. I understand that this feature was broken for months, but now that we know about it, we should fix it according to our lifecycle policy.

In such case, this should be good enough. I set the date to 2026 but we should upgrade to 3007.x before that. LMK if you'd prefer to set a different date.

We won't move to the 3007 series, that's a short-term-support release from upstream. They won't support it for as long as they will support 3006 and 3008, by skipping it we can benefit from reduced backporting efforts. 2026 should be okay.

Question: shall I also adjust https://github.com/openSUSE/salt/blob/openSUSE/release/3006.0/salt/modules/aptpkg.py#L3130 ? We adjusted the date to 2025, but I'm quite sure we won't update to 3007.x by Jan 1st, 2025, right?

We should start packaging 3008 soon, but we won't replace 3006 by January, that's correct. We should either bump this date, or change warn_until_date to never raise an exception.

@m-czernek
Copy link
Contributor Author

... now that we know about it, we should fix it according to our lifecycle policy.

Ack, makes sense.

We should either bump this date, or change warn_until_date to never raise an exception.

Personally, I'd prefer to adjust the date as that change is simpler (both to understand and to implement) than modifying the warn_until, warn_until_date, and kwargs_warn_until methods. Done in a new commit btw.

@m-czernek m-czernek changed the title Remove deprecated code Fix deprecated code Sep 4, 2024
@m-czernek m-czernek merged commit d5f3df0 into openSUSE:openSUSE/release/3006.0 Sep 4, 2024
8 checks passed
@m-czernek m-czernek deleted the backport-django-returner-removal branch September 4, 2024 11:11
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.

2 participants