ManageIQ / manageiq-providers-kubernetes

ManageIQ plugin for the Kubernetes provider.
https://kubernetes.io/
Apache License 2.0
7 stars 63 forks source link

Add annotations to container summary pages #530

Closed liu-samuel closed 3 months ago

liu-samuel commented 4 months ago

Add parse_annotations method and add them to custom attributes of container summary pages that have labels

Relevant PRs: UI-Classic: https://github.com/ManageIQ/manageiq-ui-classic/pull/9214 Providers-Openshift: https://github.com/ManageIQ/manageiq-providers-openshift/pull/262 Core ManageIQ: https://github.com/ManageIQ/manageiq/pull/23074

Cross-Repo Tests: https://github.com/ManageIQ/manageiq-cross_repo-tests/pull/879 All passing except for known failing test cases in core ManageIQ

agrare commented 4 months ago

This looks great, if we already have annotations in the VCR cassettes it would be great to write a spec test confirming that we are setting the annotations on the records that get created.

Don't worry about the rubocop warnings, they used to recommend %w() then changed to %w[] and we haven't updated all of our code.

agrare commented 4 months ago

So it looks like we have some pod annotations in the current VCRs which is great "AG parse_pod: annotations: [[{:section=>\"annotations\", :name=>\"kubernetes.io/created-by\", :value=>\"{\\\"kind\\\":\\\"SerializedReference\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"reference\\\":{\\\"kind\\\":\\\"ReplicationController\\\",\\\"namespace\\\":\\\"default\\\",\\\"name\\\":\\\"monitoring-heapster-controller\\\",\\\"uid\\\":\\\"1f2d2157-35f2-11e5-8917-001a4a5f4a00\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"resourceVersion\\\":\\\"100\\\"}}\", :source=>\"kubernetes\"}]]"

We can add an expect around [here] to check that the annotations are saved correctly.

liu-samuel commented 4 months ago

So it looks like we have some pod annotations in the current VCRs which is great "AG parse_pod: annotations: [[{:section=>\"annotations\", :name=>\"kubernetes.io/created-by\", :value=>\"{\\\"kind\\\":\\\"SerializedReference\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"reference\\\":{\\\"kind\\\":\\\"ReplicationController\\\",\\\"namespace\\\":\\\"default\\\",\\\"name\\\":\\\"monitoring-heapster-controller\\\",\\\"uid\\\":\\\"1f2d2157-35f2-11e5-8917-001a4a5f4a00\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"resourceVersion\\\":\\\"100\\\"}}\", :source=>\"kubernetes\"}]]"

We can add an expect around [here] to check that the annotations are saved correctly.

Hey is there a way I could see the current VCRs so that I know what values to test for each summary page? Also I think my CI tests are failing because my core ManageIQ PR needs to be merged first in order for annotations to be recognized as a custom attribute.

agrare commented 4 months ago

@miq-bot cross-repo-tests https://github.com/ManageIQ/manageiq/pull/23074, https://github.com/ManageIQ/manageiq-providers-openshift/pull/262

agrare commented 4 months ago

Hey @liu-samuel, cross-repo-tests is a way to test PRs with other dependent PRs so I've requested one that includes your Core PR and the Openshift one.

https://github.com/ManageIQ/manageiq-cross_repo-tests/pull/874

agrare commented 4 months ago

@liu-samuel here are the VCR cassettes https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/master/spec/vcr_cassettes/manageiq/providers/kubernetes/container_manager/refresher.yml

liu-samuel commented 4 months ago

Hey @agrare, where did you find this?

"AG parse_pod: annotations: [[{:section=>\"annotations\", :name=>\"kubernetes.io/created-by\", :value=>\"{\\\"kind\\\":\\\"SerializedReference\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"reference\\\":{\\\"kind\\\":\\\"ReplicationController\\\",\\\"namespace\\\":\\\"default\\\",\\\"name\\\":\\\"monitoring-heapster-controller\\\",\\\"uid\\\":\\\"1f2d2157-35f2-11e5-8917-001a4a5f4a00\\\",\\\"apiVersion\\\":\\\"v1\\\",\\\"resourceVersion\\\":\\\"100\\\"}}\", :source=>\"kubernetes\"}]]"

When I was looking through the file you linked, I was only able to see

http://"annotations":{"kubernetes.io/created-by":"{\"kind\":\"SerializedReference\",\"apiVersion\":\"v1\",\"reference\":{\"kind\":\"ReplicationController\",\"namespace\":\"default\",\"name\":\"monitoring-heapster-controller\",\"uid\":\"1f2d2157-35f2-11e5-8917-001a4a5f4a00\",\"apiVersion\":\"v1\",\"resourceVersion\":\"100\"}}"}},

It seems like they are the same thing but I was just curious how you were able to find the Ruby hash version where each symbol mapped to a value, like for example :name=>\"kubernetes.io/created-by\" and where :source=>\"kubernetes\" came from.

I'm also a bit confused as to why the tests here are failing, https://github.com/ManageIQ/manageiq-cross_repo-tests/pull/879 It seems like the tests for core ManageIQ that are failing aren't related to the code changes that I made.

liu-samuel commented 4 months ago

@miq-bot cross-repo-tests https://github.com/ManageIQ/manageiq/pull/23074, https://github.com/ManageIQ/manageiq-providers-openshift/pull/262 https://github.com/ManageIQ/manageiq-ui-classic/pull/9214

agrare commented 4 months ago

@liu-samuel Hey FYI if you want to restart a cross-repo-test because you pushed some new changes you should be able to just close&reopen the PR vs creating a new one

miq-bot commented 4 months ago

Checked commit https://github.com/liu-samuel/manageiq-providers-kubernetes/commit/6a06c51c3a59bbf9ea3497fed584985049ae05d9 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 3 files checked, 4 offenses detected

app/models/manageiq/providers/kubernetes/inventory/persister/definitions/container_collections.rb

agrare commented 3 months ago

Cross-repo tests are green