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
remove hard coded model versions from kubevirt #9349
remove hard coded model versions from kubevirt #9349
Conversation
please review @christianvogt @vojtechszocs |
/retest |
5503d58
to
70da810
Compare
/retest |
import { Extension, ExtensionDeclaration, CodeRef } from '../types'; | ||
|
||
/** YAML templates for editing resources via the yaml editor. */ | ||
export type YAMLTemplate = ExtensionDeclaration< | ||
'console.yaml-template', | ||
{ | ||
/** Model associated with the template. */ | ||
model: ExtensionK8sModel; | ||
model: ExtensionK8sGroupKindModel; |
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.
It seems odd to me that a yaml template isn't tied a version.
cc @spadgett
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.
why not use the dynamic version which is available in the cluster?
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.
why not use the dynamic version which is available in the cluster?
They're not API compatible. Different API versions need different YAML templates due to API differences.
The API version should be part of the model, though, right?
"/k8s/ns/:ns/kubevirt.io~v1~VirtualMachine/:name", | ||
"/k8s/ns/:ns/kubevirt.io~v1alpha3~VirtualMachine/:name" | ||
], | ||
"path": ["/k8s/ns/:ns/virtualmachines/:name"], |
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.
Is this for backwards compatibility?
All links in the app should be using the qualified reference
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.
its only for backwards, there is no link that is using this url in the app.
some tests however need this url
"kind": "VirtualMachine", | ||
"version": "v1alpha3" | ||
"kind": "VirtualMachine" |
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'd prefer to contribute separate templates per version.
When we adopt typescript definitions of the console extensions, you can reuse the same function to generate the the yaml.
export const modelForGroupKind = (group: string, kind: string): K8sKind => { | ||
const models: ImmutableMap<string, K8sKind> = store.getState().k8s.getIn(['RESOURCES', 'models']); |
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.
We want to avoid using the global redux store.
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.
well Vojtech already approved this change so it got in inside #9355
would like to hear if there's another solution that is viable for both React and non-React context
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.
If we need data from application's Redux store outside a React component, it should be OK to use store.getState()
.
I think @christianvogt is referring to the import store
declaration which adds dependency on public/redux.ts
module, we can refactor the code to avoid that static import if needed.
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.
the import store
has been inside this file for over 4 years :)
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.
No my comment was about the new use of store.getState()
.
I don't want to see more use cases that use the global store because it's bad practice.
Yes the import has been around a while now because removing these instances is difficult.
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.
If we need data from application's Redux store outside a React component, it should be OK to use store.getState()
@christianvogt I see no other way here when dealing with contexts that are out of React.
Can you explain what are your concerns here? other than the fact it doesn't look nice
70da810
to
bb0e506
Compare
bb0e506
to
a54479f
Compare
a54479f
to
b959042
Compare
b959042
to
8a1180d
Compare
@christianvogt @spadgett reverted yaml templates changes |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, glekner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
JIRA: https://issues.redhat.com/browse/CNV-12565
version
in the YAML Template extensionmodelForGroupKind
method to find a model through API Discovery bygroup
andkind
only