Deprecation Warnings - how to handle better

We noticed in Katello that we will deprecate functionality and forget to actually remove the deprecated functionality. :flushed: Here is an issue with some pretty egregious examples. Deprecated functionality from 3.5 still exists in the application today (we are on 3.15).

This doesn’t reflect well on us nor inspires a lot of confidence from our users. I am taking a look at adding testing for deprecation warnings, I have a draft PR up here: https://github.com/Katello/katello/pull/8554

This PR refactors the warnings so that they can be iterated through and the versions checked against the Katello version. This means when the release manager bumps the release, the tests will fail if the current Katello version is greater or equal to the intended deprecation version.

My question is: Has Foreman and/or other plugins seen the same problem and if so, should we add this functionality to Foreman?

We could make this functionality available to plugins and add this in a non-breaking way. I have some ideas on implementation, but want to make sure its a wanted feature in Foreman + plugins before adding it in Foreman.

Let me know if you have any questions, looking forward to some feedback :slight_smile:

I’ve noticed the same and we can definitely do better though I can’t speak for Foreman itself, or its other plugins aside from Katello.

Thank you for illustrating how this might work in your PR: it’s a straightforward pattern and I agree with the ‘registry’ approach. I would strongly recommend adding this directly in Foreman, and I hope others will agree. It would not break existing deprecation warnings while providing a standard mechanism for tracking deprecations across the whole project.

Assuming this makes it into Foreman I think it would be OK to leave the failing test behavior out of the picture - individual plugins can determine whether they want to do that or not. I wouldn’t mind having it in Katello or even delegating the auditing to something like tool_belt.

1 Like

I just want to throw in that as_deprecation_tracker could also be used without further changes, as https://github.com/Katello/katello/pull/8486 shows. If those message are added to Katello’s as_deprecation_whitelist.yaml, the tests would still be green.

After each release branching, the related messages could get deleted from the yaml, so all places that use those deprecated functions would show up in the test run again.

Do I understand the gem correctly that it tracks deprecation from Rails (and looks like from Foreman core as well in Katello’s case) and fails tests if they aren’t whitelisted? I could see Katello adding it being useful though, I appreciate you pointing that out and the PR.

I’m not sure it would solve the problem of us forgetting to remove our own deprecation warnings and associated functionality when we say we will, so there still needs to be a solution there afaict. I don’t think you meant using as_deprecation_tracker as a solution for the original issue, but just want to make sure I understand correctly.

Not 100% sure if I’m understanding the issue correctly, but I’m saying it could be used (without an automatic failure at version bumping, though), see i.e. https://github.com/theforeman/foreman/commit/6bbaa999bc1ba20ffa45c289a331485a75ddbbc9#diff-cdc38107a5bd76d32fc9a8b3b2f7478e

I believe it’s woth contribution to core. :slight_smile:

1 Like

The behavior we are seeing is:

  • Set a deprecation warning that A will be removed in B release (usually in two y-stream releases from the current one, one to deprecate, one to remove)
  • When B release comes, A is not actually removed as no one remembers to remove it months later
  • A sits in our codebase with a deprecation warning referencing an old release

The expected behavior being A should have been removed in B release like we said to users.

Some time ago I saw interesting gem solving the same problem: https://github.com/searls/todo_or_die

We also have a similar issue in foreman sometimes, but generally removing deprecations is something that is done as part of the branching process. there is even a checklist item for it.
TBH, I don’t think it’s a big deal if we remove code sometimes a version or two later than we said we would, but ideally at least once every other version the release manager will go over the code and clean up all deprecations that should have been removed. Usually it is a matter of running a search for “deprecat” and removing a few lines, though there are sometimes more complex ones.
Perhaps though the correct way of making sure deprecated code is removed would be to just create redmine issues with the correct target version for the removal? That way when going over issues targeted for the release we will notice the removal ones. The only downside here is that we normally don’t create new releases in redmine for versions beyond current nightly, we could do that, though it would require a bit of change in the automation (currently when merging issues are marked as fixed in the newest available version).

It may not seem like a big deal, but I think we should start taking this kind of thing more seriously. If a user is using software that says something will be deprecated in N and they are on N+1 or later, they will start to question anything else the application or the development team is telling them. Its breaking a unspoken contract between the user and the application.

We could go this route, but it does require the developer actually making a redmine issue for versions later. I’m trying to get to the point where the developer creates a deprecation warning, mentions a target deprecation release in it, and then 3-6 months later gets a ping from the release manager that a deprecation warning is now outdated. They can then open up a PR to remove the deprecated functionality.

I think this kind of check would be best to do in our release process and live in our release tooling. I made a PR to toolbelt for a command to parse the code of a repository and look for outdated deprecation as a step we can add to the release process https://github.com/theforeman/tool_belt/pull/237

This was merged and you can now check for deprecation warnings using toolbelt with ./tools.rb check-deprecation-warnings configs/myapp/1.0.yaml. We’ve added the step in the Katello procedures and I would encourage Foreman and plugins to try it out so we can make improvements and see how we can further improve our deprecation warning handling.

Thanks for the feedback all!

1 Like