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 custom attributes on summary pages for routes and templates #262

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

liu-samuel
Copy link
Contributor

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

Relevant PRs:
Providers-Kubernetes: ManageIQ/manageiq-providers-kubernetes#530
UI-Classic: ManageIQ/manageiq-ui-classic#9214
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

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2024

Should this be pushed down to the kubernetes layer? Or is this a specific feature of OpenShift?

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2024

Additionally, I'm not sure smashing together annotations and attributes makes sense, but I like the idea of at least putting them under a separate key.

Annotations are pure metadata whereas labels have a functional usage in kubernetes (that is, they are the other half of selectors).

So, my original thought was separate modeling, even though technically they are just more key/value pairs at the raw data level. @agrare Would like your thoughts here.

@agrare
Copy link
Member

agrare commented Jun 28, 2024

Should this be pushed down to the kubernetes layer? Or is this a specific feature of OpenShift?

Routes and ContainerTemplates don't exist in kubernetes so that's why these parsers are here in the Openshift Parser Subclass

@agrare
Copy link
Member

agrare commented Jun 28, 2024

Additionally, I'm not sure smashing together annotations and attributes makes sense, but I like the idea of at least putting them under a separate key.

Labels / Selectors / AdditionalAttributes are all CustomAttributes with a different "section", see https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/530/files#r1657929376

So I don't think this is "smashing" together annotations and attributes since they are kept separate unless I'm missing something.

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2024

Ohh I see - I thought labels were just top level, but if they are all sectioned out, then this is great.

@agrare
Copy link
Member

agrare commented Jun 28, 2024

Yeah I did a double-take when I saw custom_attributes(container_group, :labels => labels, :node_selectors => node_selector_parts, :annotations => annotations) in the kubernetes PR and had to see what the custom_attributes method did.

It might be better if custom_attributes took a section argument instead of one big hash that is splits up internally but that could be a nice follow-up after these changes.

@liu-samuel liu-samuel force-pushed the annotations-on-summary-page branch from 2416231 to ad5ff80 Compare July 3, 2024 15:45
@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2024

Checked commit liu-samuel@ad5ff80 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare closed this Jul 10, 2024
@agrare agrare reopened this Jul 10, 2024
@agrare agrare merged commit 91d1f04 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