Refactor inventory plugin #217

fabianvf · 2020-09-02T18:18:28Z

SUMMARY

Inventory plugin only adds valid hosts and groups

No longer adds services to lists of hosts
Group names are now sanitized
Removed the *_pods groups, as pods are the only valid hosts anyway
Add new groups for Deployments, Daemonsets and StatefulSets, allowing
easy access to pods that are created by those resources
Move extract_selector method to common

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

plugins/inventory/k8s.py

ADDITIONAL INFORMATION

Technically modifying the group names to be valid could be considered a breaking change, though it's probably a bug that they weren't in the first place.

codecov · 2020-09-02T18:22:14Z

Codecov Report

Merging #217 (bafc601) into main (c9157ce) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #217   +/-   ##
=======================================
  Coverage   23.17%   23.17%           
=======================================
  Files           1        1           
  Lines         151      151           
  Branches       24       24           
=======================================
  Hits           35       35           
  Misses        111      111           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9157ce...bafc601. Read the comment docs.

Akasurde · 2020-09-04T07:54:39Z

plugins/inventory/k8s.py

        return host.replace('https://', '').replace('http://', '').replace('.', '-').replace(':', '_')

+    @staticmethod
+    def sanitize(name):


There is a function called _sanitize_group_name. Can we use this?

awesome, yeah we can definitely use that

geerlingguy · 2020-09-08T15:22:42Z

I like:

No longer adds services to lists of hosts
Removed the *_pods groups, as pods are the only valid hosts anyway

And I have never used those features (and have only tested the inventory plugin once anyways), but if we merge those changes, we will need to bump to a 2.0.0 version.

We could merge the rest of the changes in 1.x, I think—and then maybe you can move those other two changes to a PR that we could put in a 2.0.0 milestone?

tima · 2020-09-08T18:22:55Z

@fabianvf I agree about the group renaming and have to wonder if many at all where using the parts you touched. A breaking change is a breaking change that I'm going to attach this one to the 2.0 project.

fabianvf · 2020-09-08T17:22:05Z

plugins/inventory/k8s.py

+                resource = client.resources.get(api_version=item['api_version'], kind=item['kind'])
+                instances = resource.get(namespace=namespace)
+            except Exception:
+                # TODO Could be expected due to RBAC or odd cluster, maybe should log a warning or something?


@geerlingguy do you know what the typical warning approach for inventory plugins is?

That I do not; you'd have to get @Akasurde's input, I have never built an inventory plugin (yet—I'm going to work on getting one merged for DigitalOcean soon).

We can use strict parameter and depend upon strictness (strict: true) we take action.

like -

if strict: raise AnsibleError("Could not... else: display.warning("Could not...

You can find the example https://github.com/ansible-collections/vmware/blob/341c3375941d3119a61c10359c85a4caa8082e18/plugins/inventory/vmware_vm_inventory.py#L446 and https://github.com/ansible-collections/vmware/blob/341c3375941d3119a61c10359c85a4caa8082e18/plugins/inventory/vmware_vm_inventory.py#L807

maxamillion · 2020-09-21T15:08:55Z

plugins/inventory/k8s.py


    description:
-      - Fetch containers and services for one or more clusters
+      - Fetch containers in pods for one or more clusters


Is this a change in functionality or is this a correction to the documentation?

That's a change in functionality; this PR would limit the scope of the inventory plugin to something that's more practical in real-world use, but because it changes the behavior and removes a feature, that should probably be put into a 2.x.

Akasurde · 2021-03-16T15:39:01Z

recheck

- No longer adds services to lists of hosts - Group names are now sanitized - Removed the `*_pods` groups, as pods are the only valid hosts anyway - Add new groups for Deployments, Daemonsets and StatefulSets, allowing easy access to pods that are created by those resources

goneri · 2021-03-29T15:41:34Z

plugins/module_utils/common.py

        result['method'] = 'create'
        return result

+    def extract_selectors(self, instance):


The method don't use self, can we convert it in a function and add test-coverage?

goneri

Looks good to me, but I still need to test the code.

tima · 2021-03-29T16:02:22Z

I don't feel we have enough feedback on the validity or necessity of this plugin. I'd like to see us hold up on this until we get that for or against it's continued inclusion.

github-actions · 2021-11-04T19:09:05Z

This repository does not accept pull requests, see the README for details.

tima requested review from Akasurde and geerlingguy September 2, 2020 18:56

tima added type/enhancement New feature or request priority/high labels Sep 2, 2020

Akasurde reviewed Sep 4, 2020

View reviewed changes

fabianvf force-pushed the refactor-inventory-plugin branch from 636c4f2 to e15ac6d Compare September 8, 2020 17:16

fabianvf changed the title ~~[WIP] Refactor inventory plugin~~ Refactor inventory plugin Sep 8, 2020

tima requested a review from willthames September 8, 2020 18:22

fabianvf commented Sep 8, 2020

View reviewed changes

tima added the lifecycle/frozen This issue is frozen for the time being and will be picked up at a later point in time label Sep 16, 2020

maxamillion reviewed Sep 21, 2020

View reviewed changes

Akasurde force-pushed the refactor-inventory-plugin branch from f81f418 to fff40ef Compare March 8, 2021 17:34

Akasurde self-assigned this Mar 8, 2021

Akasurde removed the lifecycle/frozen This issue is frozen for the time being and will be picked up at a later point in time label Mar 8, 2021

Akasurde force-pushed the refactor-inventory-plugin branch 4 times, most recently from c3c056e to 0accff9 Compare March 24, 2021 09:38

fabianvf added 5 commits March 29, 2021 11:28

Move extract_selector method to common

ea38414

Update molecule to use one of the new groups

d0f50b9

Make parent resources more easily configurable

3dfc221

use builtin sanitize function

c9462d5

Akasurde force-pushed the refactor-inventory-plugin branch 3 times, most recently from d89948c to 9f51454 Compare March 29, 2021 11:36

Akasurde force-pushed the refactor-inventory-plugin branch from 9f51454 to efa80d3 Compare March 29, 2021 12:52

Remove breaking changes

bafc601

Akasurde force-pushed the refactor-inventory-plugin branch from efa80d3 to bafc601 Compare March 29, 2021 14:43

goneri reviewed Mar 29, 2021

View reviewed changes

goneri approved these changes Mar 29, 2021

View reviewed changes

tima mentioned this pull request Apr 8, 2021

How is the k8s inventory plugin being used? ansible-collections/kubernetes.core#31

Closed

Akasurde closed this Jun 7, 2021

github-actions bot added the repo-lockdown label Nov 4, 2021

github-actions bot locked and limited conversation to collaborators Nov 4, 2021

Refactor inventory plugin #217

Refactor inventory plugin #217

Uh oh!

Conversation

fabianvf commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

codecov bot commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geerlingguy commented Sep 8, 2020

Uh oh!

tima commented Sep 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Akasurde commented Mar 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goneri left a comment

Choose a reason for hiding this comment

Uh oh!

tima commented Mar 29, 2021

Uh oh!

github-actions bot commented Nov 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fabianvf commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading