search

ansible-collections / community.kubernetes

Kubernetes Collection for Ansible
https://galaxy.ansible.com/community/kubernetes
GNU General Public License v3.0
265 stars 106 forks source link

Fix apis being overwritten when using k8s_cluster_info #389

Closed abikouo closed 3 years ago

abikouo commented 3 years ago
SUMMARY

Apis being overwritten when multiple values on the same group Fix #380

ISSUE TYPE
COMPONENT NAME

k8s_cluster_info

ADDITIONAL INFORMATION
{
  "": {
            "api_version": "v1",
            "categories": [],
            "kind": "Binding",
            "name": "bindings",
            "namespaced": true,
            "preferred": true,
            "short_names": [],
            "singular_name": "binding"
     },
    "admissionregistration.k8s.io": {
            "api_version": "admissionregistration.k8s.io/v1",
            "categories": [],
            "kind": "ValidatingWebhookConfiguration",
            "name": "validatingwebhookconfigurations",
            "namespaced": false,
            "preferred": true,
            "short_names": [],
            "singular_name": "validatingwebhookconfiguration"
     },
    "apiextensions.k8s.io": {
            "api_version": "apiextensions.k8s.io/v1",
            "categories": [],
            "kind": "CustomResourceDefinition",
            "name": "customresourcedefinitions",
            "namespaced": false,
            "preferred": true,
            "short_names": [
                "crd",
                "crds"
            ],
            "singular_name": "customresourcedefinition"
     },
     [...]
}

output after change

  {
       "admissionregistration.k8s.io/v1": {
             "MutatingWebhookConfiguration": {
                "categories": [],
                "name": "mutatingwebhookconfigurations",
                "namespaced": false,
                "preferred": true,
                "short_names": [],
                "singular_name": "mutatingwebhookconfiguration"
             },
             "ValidatingWebhookConfiguration": {
                "categories": [],
                "name": "validatingwebhookconfigurations",
                "namespaced": false,
                "preferred": true,
                "short_names": [],
                "singular_name": "validatingwebhookconfiguration"
              }
        },
        "admissionregistration.k8s.io/v1beta1": {
              "MutatingWebhookConfiguration": {...},
              "ValidatingWebhookConfiguration": {...}
        },
       (...)
        "v1": {
              "Binding": {...},
              "ComponentStatus": {...},
             (...)
              "PersistentVolumeClaim": {...},
              "Pod": {...},
              "PodTemplate": {...},
              "ReplicationController": {...},
       }
}
codecov[bot] commented 3 years ago

Codecov Report

Merging #389 (35cc336) into main (c9157ce) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #389   +/-   ##
=======================================
  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...35cc336. Read the comment docs.

Akasurde commented 3 years ago

@abikouo Thanks for the PR. Could you please paste return info before and after?

abikouo commented 3 years ago

Hi @Akasurde

The return info before the commit was similar to this 

{
  "": {
            "api_version": "v1",
            "categories": [],
            "kind": "Binding",
            "name": "bindings",
            "namespaced": true,
            "preferred": true,
            "short_names": [],
            "singular_name": "binding"
     },
    "admissionregistration.k8s.io": {
            "api_version": "admissionregistration.k8s.io/v1",
            "categories": [],
            "kind": "ValidatingWebhookConfiguration",
            "name": "validatingwebhookconfigurations",
            "namespaced": false,
            "preferred": true,
            "short_names": [],
            "singular_name": "validatingwebhookconfiguration"
     },
    "apiextensions.k8s.io": {
            "api_version": "apiextensions.k8s.io/v1",
            "categories": [],
            "kind": "CustomResourceDefinition",
            "name": "customresourcedefinitions",
            "namespaced": false,
            "preferred": true,
            "short_names": [
                "crd",
                "crds"
            ],
            "singular_name": "customresourcedefinition"
     },
     [...]
}

now the return info will look like this 

{
"": [
        {
            "api_version": "v1",
            "categories": [],
            "kind": "Binding",
            "name": "bindings",
            "namespaced": true,
            "preferred": true,
            "short_names": [],
            "singular_name": "binding"
        },
        {
            "api_version": "v1",
            "categories": [],
            "kind": "ComponentStatus",
            "name": "componentstatuses",
            "namespaced": false,
            "preferred": true,
            "short_names": [
                "cs"
            ],
            "singular_name": "componentstatuse"
        },
       [...]
    ],
    "admissionregistration.k8s.io": [
        {
            "api_version": "admissionregistration.k8s.io/v1",
            "categories": [],
            "kind": "MutatingWebhookConfiguration",
            "name": "mutatingwebhookconfigurations",
            "namespaced": false,
            "preferred": true,
            "short_names": [],
            "singular_name": "mutatingwebhookconfiguration"
        },
        {
            "api_version": "admissionregistration.k8s.io/v1",
            "categories": [],
            "kind": "ValidatingWebhookConfiguration",
            "name": "validatingwebhookconfigurations",
            "namespaced": false,
            "preferred": true,
            "short_names": [],
            "singular_name": "validatingwebhookconfiguration"
        },
        [...]
    ],
    "apiextensions.k8s.io": [
        {
            "api_version": "apiextensions.k8s.io/v1",
            "categories": [],
            "kind": "CustomResourceDefinition",
            "name": "customresourcedefinitions",
            "namespaced": false,
            "preferred": true,
            "short_names": [
                "crd",
                "crds"
            ],
            "singular_name": "customresourcedefinition"
        },
        {
            "api_version": "apiextensions.k8s.io/v1beta1",
            "categories": [],
            "kind": "CustomResourceDefinition",
            "name": "customresourcedefinitions",
            "namespaced": false,
            "preferred": false,
            "short_names": [
                "crd",
                "crds"
            ],
            "singular_name": "customresourcedefinition"
        }
    ],
    [...]
}
Akasurde commented 3 years ago

@abikouo @tima @gravesm I think this is a "breaking change". Previously we returned dict whereas now this change return list inside a dict.

gravesm commented 3 years ago

Yes, this would be a breaking change. Also, we should add some tests for this, or better yet, just fix the test in https://github.com/ansible-collections/community.kubernetes/blob/main/molecule/default/tasks/cluster_info.yml. That test failed to catch this bug and is also failing to catch this breaking change, which says to me it's not a very good test.

abikouo commented 3 years ago

@Akasurde I totally agree with you, in fact there were 2 proposals to solve this issue, either returning items as on list per group (this is what has been done here) or change the key of the return dictionary in order to avoid overriding items

tima commented 3 years ago

Good point @Akasurde. I think if we wanted to introduce this breaking changing doing it as part of the v2.0 effort would be a good time. We'll need to make sure to clearly document this break and promote it perhaps as some sort of porting guidance section.

abikouo commented 3 years ago

@gravesm please have a look. WDYT

gravesm commented 3 years ago

I'm :+1: on @fabianvf's suggestion of group+version+kind.

abikouo commented 3 years ago

I'm on @fabianvf's suggestion of group+version+kind.

Hey @gravesm
the pull request has been updated accordingly, could you please review ? Thanks

abikouo commented 3 years ago

migrated to kubernetes.core

github-actions[bot] commented 2 years ago

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