squakez commented on code in PR #6807:
URL: https://github.com/apache/camel-k/pull/6807#discussion_r4006382267
##########
pkg/apis/camel/v1/common_types_support_test.go:
##########
@@ -35,7 +36,7 @@ func TestTraitsMerge(t *testing.T) {
Auto: ptr.To(false),
ServicePort: 81,
},
- Logging: &trait.LoggingTrait{
+ DeprecatedLogging: &trait.LoggingTrait{
Review Comment:
We better use another trait to test the merge. If we can do in this PR
that's great, otherwise it can be a follow up PR.
##########
pkg/apis/camel/v1/common_types_support_test.go:
##########
@@ -61,9 +62,23 @@ func TestTraitsMerge(t *testing.T) {
assert.Equal(t, int32(81), t1.Container.ServicePort)
// values from merged trait take precedence over the original ones
- assert.NotNil(t, t1.Logging)
- assert.True(t, ptr.Deref(t1.Logging.Color, false))
- assert.Equal(t, "DEBUG", t1.Logging.Level)
+ assert.NotNil(t, t1.DeprecatedLogging)
+ assert.True(t, ptr.Deref(t1.DeprecatedLogging.Color, false))
+ assert.Equal(t, "DEBUG", t1.DeprecatedLogging.Level)
+}
+
+func TestDeprecatedTraitsJSONNames(t *testing.T) {
Review Comment:
Let's remove this, it really adds nothing.
##########
pkg/trait/trait_configure.go:
##########
@@ -88,6 +88,8 @@ func (c *Catalog) configureTrait(id string, trait
map[string]any) error {
if err := decodeTrait(trait, catTrait); err != nil {
return err
}
+ } else if len(trait) > 0 {
Review Comment:
Why this change here? I think we can silently ignore the configuration
without bloating the log.
##########
pkg/trait/util.go:
##########
@@ -356,6 +365,15 @@ func ExtractAndMaybeDeleteTraits(c client.Client,
annotations map[string]string,
return &traits, nil
}
+func isRemovedTrait(id string) bool {
Review Comment:
This is not needed. We have many removed traits and we cannot control them.
##########
pkg/trait/util_test.go:
##########
@@ -256,6 +256,56 @@ func TestIntegrationAndPipeSameTraits(t *testing.T) {
assert.True(t, result)
}
+func TestExtractAndMaybeDeleteRemovedTraitAnnotations(t *testing.T) {
+ for _, traitID := range []string{"logging", "master", "telemetry"} {
+ t.Run(traitID, func(t *testing.T) {
+ client, err := internal.NewFakeClient()
+ require.NoError(t, err)
+
+ annotation := v1.TraitAnnotationPrefix + traitID +
".enabled"
+ annotations := map[string]string{annotation: "true"}
+
+ traits, err := ExtractAndMaybeDeleteTraits(client,
annotations, true)
+ require.NoError(t, err)
+ assert.Nil(t, traits)
+ assert.NotContains(t, annotations, annotation)
+ })
+ }
+}
+
+func TestNewSpecTraitsOptionsIgnoreRemovedTraitAnnotations(t *testing.T) {
Review Comment:
Remove
##########
pkg/trait/trait_configure_test.go:
##########
@@ -162,3 +160,44 @@ func TestTraitSplitConfiguration(t *testing.T) {
ot, _ := c.GetTrait("owner").(*ownerTrait)
assert.Equal(t, []string{"opt1", "opt2"}, ot.TargetLabels)
}
+
+func TestRemovedTraitConfiguration(t *testing.T) {
Review Comment:
I'd avoid this test. The fact that that trait logic is missing should be
enough to confirm these traits cannot be executed.
##########
pkg/trait/util.go:
##########
@@ -330,19 +330,28 @@ func ExtractAndMaybeDeleteTraits(c client.Client,
annotations map[string]string,
traitsPlainParams := []string{}
for k, v := range annotations {
//nolint:staticcheck
- if strings.HasPrefix(k, v1.TraitAnnotationPrefix) {
Review Comment:
Why this change? I think we need to revert and keep the same logic as
before. We don't want any special treatment to removed traits logic.
##########
pkg/controller/pipe/integration_test.go:
##########
@@ -294,6 +294,23 @@ func TestExtractTraitAnnotations(t *testing.T) {
assert.Equal(t, "hello", annotations["my-personal-annotation"])
}
+func TestExtractRemovedTraitAnnotations(t *testing.T) {
Review Comment:
No need for this test IMO
##########
pkg/trait/util_test.go:
##########
@@ -256,6 +256,56 @@ func TestIntegrationAndPipeSameTraits(t *testing.T) {
assert.True(t, result)
}
+func TestExtractAndMaybeDeleteRemovedTraitAnnotations(t *testing.T) {
Review Comment:
Remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
