We noticed in Katello that we will deprecate functionality and forget to actually remove the deprecated functionality. 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 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
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.
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.
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.
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).