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

Add annotations to container summary pages #530

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

liu-samuel
Copy link
Contributor

@liu-samuel liu-samuel commented Jun 27, 2024

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

Relevant PRs:
UI-Classic: ManageIQ/manageiq-ui-classic#9214
Providers-Openshift: ManageIQ/manageiq-providers-openshift#262
Core ManageIQ: ManageIQ/manageiq#23074

Cross-Repo Tests:
ManageIQ/manageiq-cross_repo-tests#879
All passing except for known failing test cases in core ManageIQ

@@ -58,23 +58,23 @@ def initialize_container_conditions
def initialize_custom_attributes
%i(container_nodes
container_projects).each do |name|
add_custom_attributes(name, %w(labels additional_attributes))
add_custom_attributes(name, %w(labels additional_attributes annotations))
Copy link
Member

Choose a reason for hiding this comment

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

I believe these have to be defined as associations on these models, e.g. https://github.com/ManageIQ/manageiq/blob/master/app/models/container_node.rb#L26-L33

@agrare
Copy link
Member

agrare commented Jun 27, 2024

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
Copy link
Member

agrare commented Jun 28, 2024

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
Copy link
Contributor Author

liu-samuel commented Jul 2, 2024

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
Copy link
Member

agrare commented Jul 2, 2024

@agrare
Copy link
Member

agrare commented Jul 2, 2024

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.

ManageIQ/manageiq-cross_repo-tests#874

@agrare
Copy link
Member

agrare commented Jul 2, 2024

@liu-samuel
Copy link
Contributor Author

liu-samuel commented Jul 2, 2024

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, ManageIQ/manageiq-cross_repo-tests#879 It seems like the tests for core ManageIQ that are failing aren't related to the code changes that I made.

@liu-samuel
Copy link
Contributor Author

@agrare
Copy link
Member

agrare commented Jul 2, 2024

@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
Copy link
Member

miq-bot commented Jul 3, 2024

Checked commit liu-samuel@6a06c51 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
Copy link
Member

agrare commented Jul 10, 2024

Cross-repo tests are green

@agrare agrare closed this Jul 10, 2024
@agrare agrare reopened this Jul 10, 2024
@agrare agrare merged commit 1f202fe into ManageIQ:master Jul 10, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants