-
Notifications
You must be signed in to change notification settings - Fork 63
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
Conversation
@@ -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)) |
There was a problem hiding this comment.
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
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 |
So it looks like we have some pod annotations in the current VCRs which is great 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. |
@miq-bot cross-repo-tests ManageIQ/manageiq#23074, ManageIQ/manageiq-providers-openshift#262 |
From Pull Request: ManageIQ/manageiq-providers-kubernetes#530
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. |
From Pull Request: ManageIQ/manageiq-providers-kubernetes#530
Hey @agrare, where did you find this?
When I was looking through the file you linked, I was only able to see
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 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. |
From Pull Request: ManageIQ/manageiq-providers-kubernetes#530
From Pull Request: ManageIQ/manageiq-providers-kubernetes#530
From Pull Request: ManageIQ/manageiq-providers-kubernetes#530
@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 |
476d03b
to
a627b21
Compare
a627b21
to
6a06c51
Compare
Checked commit liu-samuel@6a06c51 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint app/models/manageiq/providers/kubernetes/inventory/persister/definitions/container_collections.rb
|
Cross-repo tests are green |
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