remove trailing group_version of trait names to align with definitions #516
Conversation
|
xref: oam-dev/spec#300 |
|
Thanks for making the change. In the spec, we may want to change the trait schema to include the version information. The "type" field would be a way to differentiate different behavior or to provide feature-rich traits later. Till we make that spec change, it is fine to update the implementation to adhere to spec. This might also break samples in this repo (https://github.com/oam-dev/samples/). |
| pub const AUTOSCALER_V1ALPHA1: &str = "auto-scaler.core.oam.dev/v1alpha1"; | ||
| pub const MANUAL_SCALER_V1ALPHA1: &str = "manual-scaler.core.oam.dev/v1alpha1"; | ||
| pub const VOLUME_MOUNTER_V1ALPHA1: &str = "volume-mounter.core.oam.dev/v1alpha1"; | ||
| pub const INGRESS_V1ALPHA1: &str = "ingress"; |
There was a problem hiding this comment.
INGRESS_V1ALPHA1 [](start = 10, length = 16)
You have to make change to trait_manager.rs - load_traits() method to remove the case statements "INGRESS", "AUTOSCALER" and others. Additionally the trait object has from_param() method that needs to be removed as well.
There was a problem hiding this comment.
Why do I need to change that? Could you explain more details?
The behavior with the change looks correct to me.
There was a problem hiding this comment.
If you look at the switch stmt, "INGRESS_V1ALPHA1" is before "INGRESS" case that is why it is working. If you change the ordering it will break. With this change, we have two case options with same value and because of case statement ordering it works in your case.
Earlier params(key-value pair) was used as the way to get input and recent change uses fixed schema (via properties). Since we are moving to fixed schema old remnants that is using params should be removed.
In reply to: 367710920 [](ancestors = 367710920)
There was a problem hiding this comment.
Hi @iyyappam
Thanks for your explanations. I got your meanings.
Moreover, would you mind tell me your Slack ID? That would be easier for me to ask you additional questions and discuss issues :)
There was a problem hiding this comment.
Thanks for making the change. Also trait objects such as ingress, autoscaler has from_param() method which are not needed anymore. You can remove that too.
In reply to: 367726505 [](ancestors = 367726505)
There was a problem hiding this comment.
For faster response please mail me at iyyappam at microsoft dot com.
In reply to: 367729724 [](ancestors = 367729724,367726505)
There was a problem hiding this comment.
has from_param() method which are not needed anymore.
done.
| pub const AUTOSCALER_V1ALPHA1: &str = "auto-scaler.core.oam.dev/v1alpha1"; | ||
| pub const MANUAL_SCALER_V1ALPHA1: &str = "manual-scaler.core.oam.dev/v1alpha1"; | ||
| pub const VOLUME_MOUNTER_V1ALPHA1: &str = "volume-mounter.core.oam.dev/v1alpha1"; | ||
| pub const INGRESS_V1ALPHA1: &str = "ingress"; |
There was a problem hiding this comment.
INGRESS_V1ALPHA1 [](start = 10, length = 16)
The earlier plan was to hold making the change for backward compatibility till v0.2 is released. I'm not sure of that state now. @suhuruli might know about it.
There was a problem hiding this comment.
Not sure what's plan here?
|
Hi @iyyappam I totally agree with you on using GVK to group and differentiate traits. Actually that was discussed on the early days: oam-dev/spec#44 . Actually we are in a period to better align with k8s conventions and we will be solving this trait GVK naming in the new proposal.
The samples do not have version appended actually. So this PR would fix the behavior: |
hongchaodeng
force-pushed
the
fix_trait
branch
from
83fa80a to
7872f6a
Compare
The trait names should align with the names in trait definitions: https://github.com/oam-dev/rudr/blob/master/charts/rudr/templates/traits.yaml