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

Deprecate nios content #2458

Merged
merged 3 commits into from
May 13, 2021

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented May 5, 2021

SUMMARY

Implements ansible-community/community-topics#13.

Closes ansible-community/community-topics#13.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

nios content

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug community_review has_issue lookup lookup plugin module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_triage net_tools plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging labels May 5, 2021
@tadeboro
Copy link
Contributor

tadeboro commented May 5, 2021

How should we deprecate the https://github.com/ansible-collections/community.general/blob/main/scripts/inventory/infoblox.py? Because I would assume we will also remote the inventory script with the c.g 5.0.0 release.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Looking good to me

@felixfontein
Copy link
Collaborator Author

@tadeboro hmm, that's a good point. Also that script depends on the module_utils we will be removing (I guess I should also mark them as deprecated in meta/runtime.yml; just pushed a commit for that), but I guess we can extend them to try to import from infoblox.nios_modules as well.

There are two problems with the script:

  1. It is not contained in infoblox.nios_modules, so removing it here will remove it from every location it currently exists.
  2. There's no way (AFAIK) to deprecate inventory scripts so that users will notice. Or is it possible to print things on stderr that users will see? (I've just asked that on #ansible-devel)

@tadeboro
Copy link
Contributor

tadeboro commented May 6, 2021

1. It is not contained in infoblox.nios_modules, so removing it here will remove it from every location it currently exists.

Since Infoblox did not migrate them when they created a new collection, I would assume that they want Ansible users to use the inventory plugin that is part of their collection. So removing it does not sound that bad to me. Also, script users had to copy it somewhere, so we cannot actually remove it entirely by dropping it from the c.g collection.

2. There's no way (AFAIK) to deprecate inventory scripts so that users will notice. Or is it possible to print things on stderr that users will see? (I've just asked that on #ansible-devel)

Since this script needs to be copied manually from the repo, I would say that maybe even a big fat warning comment at the top of the file should be enough. We cannot do much for existing users since they most likely have their own copy of the script that will not notice anything new unless they update their script.

@felixfontein
Copy link
Collaborator Author

I've started ansible-community/community-topics#16 since TBH I'd like to get rid of all these scripts from this repository ;-)

@felixfontein
Copy link
Collaborator Author

felixfontein commented May 11, 2021

If nobody minds I'll merge this in a couple of days. We can deprecate the inventory plugin script also later :)

@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label May 11, 2021
Copy link
Contributor

@tadeboro tadeboro left a comment

Choose a reason for hiding this comment

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

I think we can sort the inventory script issue in a follow-up PR.

@felixfontein felixfontein merged commit ee9770c into ansible-collections:main May 13, 2021
@patchback
Copy link

patchback bot commented May 13, 2021

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/ee9770cff720259cb781f0b1d9705f33a5d83fb1/pr-2458

Backported as #2504

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request May 13, 2021
* Deprecate nios content.

* Make 2.9's ansible-test happy.

* Add module_utils deprecation.

(cherry picked from commit ee9770c)
@felixfontein
Copy link
Collaborator Author

@tadeboro @russoz thanks for reviewing this!

@felixfontein felixfontein deleted the deprecate-nios branch May 13, 2021 19:51
felixfontein added a commit that referenced this pull request May 14, 2021
* Deprecate nios content.

* Make 2.9's ansible-test happy.

* Add module_utils deprecation.

(cherry picked from commit ee9770c)

Co-authored-by: Felix Fontein <felix@fontein.de>
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review has_issue lookup lookup plugin module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_triage net_tools plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate community.general's nios modules/plugins
4 participants