Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Proposal: Remove flat-mapping from community.general and community.network #147

Closed
felixfontein opened this issue Oct 9, 2022 · 18 comments

Comments

@felixfontein
Copy link
Contributor

Summary

Right now community.general and community.network use flatmapping, which means that while modules are meant to be in the plugins/modules/ directory, they are effectively placed in subdirectories of plugins/modules/. This is basically what was done in Ansible 2.9 and before (these versions had a subdirectory structure in lib/ansible/modules/). This subdirectory structure is meant to be an implementation detail and can change at any moment.

Since community.general 5.0.0 and community.network 4.0.0, the collections no longer use symlinks to implement this, but instead use meta/runtime.yml redirects. While this has a large improvment on the footprint of the Ansible package when installed (since Python packaging converts symlinks to copies, the modules were contained twice in the Ansible tarball and thus also the installed version), this has two large downsides: 1) when running ansible-doc -l community.general (or the analogue for community.network), the directory structure is exposed in the list of modules; 2) the VScode extension actively (!) suggests users to use the internal names (ansible/vscode-ansible#573). Especially the latter is fatal, since this directory structure is considered an implenentation detail by the collections, and can thus change at any moment.

These problems lead me to reconsider whether we should keep using flatmapping. Originally there apparently was some discussion of supporting flatmapping natively by ansible-core - see for example https://github.com/ansible-collections/community.general/blob/aebc1b03fdc1a5e56f7e7bf34784f8b1d83174e2/galaxy.yml#L28 -, but core decided to not implement that for performance reasons. So basically what community.general and community.network are doing is a workaround for that missing feature.

Advantages of flatmapping:

Disadvantages:

So basically this is nice for the developers, but sucks for the users.

Since the impact for developers is IMO much less bad than the one for users, I'm for abolishing flatmapping. What do others think?

Additional Information

No response

@felixfontein felixfontein added discussion_topic next_meeting Topics that needs to be discussed in the next Community Meeting labels Oct 9, 2022
@briantist
Copy link

The changelog grouping really is nice.. but it does seem like the burden to maintain flatmapping is too high to justify it.

@felixfontein
Copy link
Contributor Author

The changelog grouping (and the not yet re-created docs grouping, as in https://docs.ansible.com/ansible/2.9/modules/modules_by_category.html / https://docs.ansible.com/ansible/2.9/modules/list_of_packaging_modules.html) could also be created from new metadata, but extracing that information from the directory structure makes it a lot easier to manage.

I've also thought about keeping the repository in the structure as-is, and on build-time moving everything around so that the redirects are no longer necessary (i.e. a pre-build preprocessing step) and that everything is actually flat. But that would add some additional complexity, and distance between the built Galaxy artefact and the repository.

One thing I forgot in the above: getting rid of flatmapping also makes adding new modules easier for first contributors, since they don't have to add the redirects to get their modules working (before 5.0.0 they had to add the symlinks, which also had its pitfalls).

@ssbarnea
Copy link
Member

ssbarnea commented Oct 10, 2022

Just to be clear, does this proposal also mean that, lets say sudoers plugin will be documented as community.general.system.sudoers instead of the current community.general.sudoers. I personally like it because it would also mean it that it would match the output of:

$ ansible-doc -F|grep sudoers
community.general.system.sudoers

Mode details on ansible/ansible-lint#2581 (reply in thread)

@felixfontein
Copy link
Contributor Author

@ssbarnea no, it means that community.general.sudoers will be documented as community.general.sudoers (as it is today), accessible as community.general.sudoers, and no longer accessible as community.general.system.sudoers (or any other internal names that were never intended to being used, and were never documented).

@felixfontein
Copy link
Contributor Author

I just found out that there is an official guideline that you should find modules by using ansible-doc --list | grep xxx (ref: ansible/ansible-lint#2581 (reply in thread)). Since ansible-doc --list is right now the only place the internal FQCNs of the modules are exposed (when not using 'external' programs like the VScode plugin), this is making the situation a lot worse.

Since it's petty unlikely that core will adjust ansible-doc to stop printing these names we consider internal, I think the only good option here is to get rid of the directory structure ASAP, preferably already for community.general 6.0.0 (obviously we need a set of (deprecated) redirects from the long names to the short names, so that folks who already started using the internal names aren't left with broken playbooks/roles).

Right now the planning for community.general is to release 5.8.0 on October 25th (+- a day maybe), and 6.0.0 on November 7th. So my proposal would be to get rid of flatmapping for 6.0.0, i.e. once 5.8.0 is out and ansible-collections/community.general#5326 is merged, remove flatmapping, and create an alpha release so we can do some testing. What do you think?

If nobody comes up with reasons why not to do this or better ideas in a couple of days, I'll start a vote on this.

@felixfontein
Copy link
Contributor Author

I did work a bit on tooling to do the heavy lifting (renaming and adjusting imports), the result is here: https://gist.github.com/felixfontein/3123dcdf446c9e140949a0589cceb99d. I've also created two PRs (ansible-collections/community.network#469, ansible-collections/community.general#5350) to clean up the unit tests to avoid file name / directory name collisions when undoing the flatmapping (in the unit test trees, there are multiple conftest.py resp. fixtures directories). With this implementing the un-flatmapping will hopefully go pretty smoothly. With these PRs, applying the un-flatmapping script works well in both community.general and community.network and at least the Python 3.10 unit tests still pass.

@ssbarnea
Copy link
Member

@felixfontein This subject took a lot of my time this week and it happened at the worst possible time (one week before fest). Still it is good that you opened the subject because we need to address it and document it properly.

@gotmax23
Copy link
Contributor

I've also thought about keeping the repository in the structure as-is, and on build-time moving everything around so that the redirects are no longer necessary (i.e. a pre-build preprocessing step) and that everything is actually flat. But that would add some additional complexity, and distance between the built Galaxy artefact and the repository.

I'm not completely opposed to that as long as it's clearly documented. However, it doesn't sound like doing so is worth it.

@felixfontein
Copy link
Contributor Author

@gotmax23 I mainly mentioned that approach for sake of completeness, I really don't like it either and would prefer not to implement it. Right now the Galaxy artefact of community.general is basically the source of the collection (except that Zuul added some additions to the recent release's artefact for whatever reason?!), and I don't really want to change that until we know how to resolve all the (potential) legal problems with this and the Ansible releases. (Also doing this unflatmapping dynamically all the time sounds pretty maintenance intensive to me...)

@felixfontein
Copy link
Contributor Author

Ref: ansible/ansible-lint#2604

@Andersson007
Copy link
Contributor

+1 to abolishing flat-mapping for the sake of users's happiness

@felixfontein
Copy link
Contributor Author

@ssbarnea any result on the discussion at AnsibleFest you mentioned in ansible/ansible-lint#2604 (comment) ? This is somewhat urgent, since we'll have to start a vote on this very soon if we want to get this done before the next major releases of c.g and c.n.

@ssbarnea
Copy link
Member

ssbarnea commented Oct 22, 2022

@felixfontein Based on the talks from Fest we will go ahead with making the canonical name recommended, canonical being what ansible-doc -l reports, the exact code path.

We know that this impacts community.general in particular and we want to wait for you to make a decision before we merge that change into the linter.

In our opinion there are two paths that makes sense to be taken. I will use the sudoers as an example as it would be very easy for others to extrapolate from it.

ansible-doc -l | grep sudoers
community.general.system.sudoers                                                    Manage sudo...

We do know that most users are using the short-FQCN community.general.sudoers as that is shorter and also advertised in docs. Docs can be updated easily to display the canonical name.

What can be done?

  • swap the module implementation with the symlink from root. This will prevent any breakage while making the canonical name become community.general.sudoers.
  • split - Make system a standalone collection. If we move community.general.system into community.system, we could have a short name such community.system.sudoers while also addressing a little bit of the the kitchen sink issue with community.general.
  • nothing or almost nothing, as once we start recommending canonical names, we will ensure that documentation does expose the canonical name instead of aliases.

My top favourite is split, followed by swap a another option. AFAIK, the number of collection using deep modules is extremely low.

@ssbarnea
Copy link
Member

I produced a blog post about upcoming canonical fqcn at https://sbarnea.com/ansible/canonical-fqcn/ which also includes some stats regarding affected collections.

@felixfontein
Copy link
Contributor Author

@felixfontein Based on the talks from Fest we will go ahead with making the canonical name recommended, canonical being what ansible-doc -l reports, the exact code path.

This is a very unfortunate decision. And it is really unfortunate that the Steering Committee was not involved in this discussion, as this impacts community a lot.

We know that this impacts community.general in particular and we want to wait for you to make a decision before we merge that change into the linter.

I guess you mean "Steering Committee" when you write "you"?

In our opinion there are two paths that makes sense to be taken. I will use the sudoers as an example as it would be very easy for others to extrapolate from it.

ansible-doc -l | grep sudoers
community.general.system.sudoers                                                    Manage sudo...

We do know that most users are using the short-FQCN community.general.sudoers as that is shorter and also advertised in docs. Docs can be updated easily to display the canonical name.

The short FQCN is the official, canonical, and only supported name of the module in the collection. The long FQCN is not canonical fom the POV of community.general.

What can be done?

* **swap** the module implementation with the symlink from root. This will prevent any breakage while making the canonical name become `community.general.sudoers`.

There are no symlinks since community.general 5.0.0. The 'swap' suggestion is basically to remove flatmapping, which this discussion issue is about.

* **split** - Make system a standalone collection. If we move `community.general.system` into `community.system`, we could have a short name such `community.system.sudoers` while also addressing a little bit of the the kitchen sink issue with `community.general`.

As mentioned on IRC, I think this is a really bad idea. This has been discussed multiple times in the past by the SC, but let me iterate again:

  • Most content in community.general is not actively maintained.
  • A lot of the content in community.general gets a drive-by contributions (bugfixes, new features).
  • These drive-by contributions need module or collection maintainers to handle them somehow (do some basic review to ensure basic standards, and merge or reject them eventually).
  • If you split up community.general into N collections, basically you need active collection maintainers for all of them to allow drive-by contributions to be handled.
  • I don't see where all these collection maintainers should come from. Some module groups or modules have more or less active maintainers, but these are usually not very active and probably don't want to step up to be a collection maintainer (and thus also being responsible for a huge amount of other modules and plugins).
  • So basically splitting up community.general into many smaller parts will produce a set of basically dead, unmaintained collections that do not allow drive-by contributions.

From my POV this is really bad for the community.

* **nothing** or almost nothing, as once we start recommending canonical names, we will ensure that documentation does expose the canonical name instead of aliases.

IMO this would be by far the worst choice.

@gotmax23
Copy link
Contributor

gotmax23 commented Nov 2, 2022

We spent the majority of today's community meeting discussing this (minutes). The attendees informally agreed (+5,0,0) to unflatmap community.general before the 2022-11-07 Ansible 7 freeze. Ideally, we would've had a full vote and more time, but the Ansible 7 freeze is in less than a week. The timing here is quite unfortunate :(. Kudos to @felixfontein for handling this on short notice!

@felixfontein
Copy link
Contributor Author

community.general 6.0.0-a1 is out with the directory structures removed. Please test whether you can see any immediate problems! There's a PR with similar changes for community.network as well: ansible-collections/community.network#482

@felixfontein
Copy link
Contributor Author

community.network 5.0.0 is out as well, this one also with a flattened directory stucture.

So I guess this problem is now resolved and we can close this. Thanks everyone!

@felixfontein felixfontein removed the next_meeting Topics that needs to be discussed in the next Community Meeting label Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants