Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j merged PR #5275: URL: https://github.com/apache/camel-k/pull/5275 -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537683483 ## pkg/trait/knative.go: ## @@ -70,7 +71,8 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if e.Integration == nil { return false, nil, nil } - if !pointer.BoolDeref(t.Enabled, true) { Review Comment: yes, I fixed it, thanks for reviewing. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537652768 ## pkg/trait/knative_test.go: ## @@ -348,6 +348,155 @@ func TestKnativePlatformHttpDependencies(t *testing.T) { } } +func TestKnativeEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('knative:channel/channel-source-1')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, + Platform: &v1.IntegrationPlatform{ + Spec: v1.IntegrationPlatformSpec{ + Cluster: v1.IntegrationPlatformClusterOpenShift, + Build: v1.IntegrationPlatformBuildSpec{ + PublishStrategy: v1.IntegrationPlatformBuildPublishStrategyS2I, + Registry: v1.RegistrySpec{Address: "registry"}, + }, + Profile: v1.TraitProfileKnative, + }, + }, + EnvVars:make([]corev1.EnvVar, 0), + ExecutedTraits: make([]Trait, 0), + Resources: k8sutils.NewCollection(), + } + environment.Platform.ResyncStatusFullConfig() + + // configure the init trait + init := NewInitTrait() + ok, condition, err := init.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the init trait + require.NoError(t, init.Apply(&environment)) + + // configure the knative trait + knTrait, _ := newKnativeTrait().(*knativeTrait) + ok, condition, err = knTrait.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the knative trait + require.NoError(t, knTrait.Apply(&environment)) + + // call the post step processors + for _, processor := range environment.PostStepProcessors { + assert.Nil(t, processor(&environment)) + } + + assert.True(t, *knTrait.Enabled) + assert.Contains(t, environment.Integration.Status.Capabilities, v1.CapabilityKnative) + assert.Contains(t, environment.Integration.Status.Dependencies, "mvn:org.apache.camel.k:camel-k-knative-impl") +} + +func TestKnativeNotEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('log:info')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, +
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
squakez commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537236203 ## pkg/trait/knative.go: ## @@ -70,7 +71,8 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if e.Integration == nil { return false, nil, nil } - if !pointer.BoolDeref(t.Enabled, true) { Review Comment: But I think that's exactly what `!pointer.BoolDeref(t.Enabled, true)` does. If the user does not set explicitly to `false`, then it is always `true`. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
squakez commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537591762 ## pkg/trait/knative_test.go: ## @@ -348,6 +348,155 @@ func TestKnativePlatformHttpDependencies(t *testing.T) { } } +func TestKnativeEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('knative:channel/channel-source-1')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, + Platform: &v1.IntegrationPlatform{ + Spec: v1.IntegrationPlatformSpec{ + Cluster: v1.IntegrationPlatformClusterOpenShift, + Build: v1.IntegrationPlatformBuildSpec{ + PublishStrategy: v1.IntegrationPlatformBuildPublishStrategyS2I, + Registry: v1.RegistrySpec{Address: "registry"}, + }, + Profile: v1.TraitProfileKnative, + }, + }, + EnvVars:make([]corev1.EnvVar, 0), + ExecutedTraits: make([]Trait, 0), + Resources: k8sutils.NewCollection(), + } + environment.Platform.ResyncStatusFullConfig() + + // configure the init trait + init := NewInitTrait() + ok, condition, err := init.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the init trait + require.NoError(t, init.Apply(&environment)) + + // configure the knative trait + knTrait, _ := newKnativeTrait().(*knativeTrait) + ok, condition, err = knTrait.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the knative trait + require.NoError(t, knTrait.Apply(&environment)) + + // call the post step processors + for _, processor := range environment.PostStepProcessors { + assert.Nil(t, processor(&environment)) + } + + assert.True(t, *knTrait.Enabled) + assert.Contains(t, environment.Integration.Status.Capabilities, v1.CapabilityKnative) + assert.Contains(t, environment.Integration.Status.Dependencies, "mvn:org.apache.camel.k:camel-k-knative-impl") +} + +func TestKnativeNotEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('log:info')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, +
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537550589 ## pkg/trait/knative_test.go: ## @@ -348,6 +348,155 @@ func TestKnativePlatformHttpDependencies(t *testing.T) { } } +func TestKnativeEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('knative:channel/channel-source-1')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, + Platform: &v1.IntegrationPlatform{ + Spec: v1.IntegrationPlatformSpec{ + Cluster: v1.IntegrationPlatformClusterOpenShift, + Build: v1.IntegrationPlatformBuildSpec{ + PublishStrategy: v1.IntegrationPlatformBuildPublishStrategyS2I, + Registry: v1.RegistrySpec{Address: "registry"}, + }, + Profile: v1.TraitProfileKnative, + }, + }, + EnvVars:make([]corev1.EnvVar, 0), + ExecutedTraits: make([]Trait, 0), + Resources: k8sutils.NewCollection(), + } + environment.Platform.ResyncStatusFullConfig() + + // configure the init trait + init := NewInitTrait() + ok, condition, err := init.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the init trait + require.NoError(t, init.Apply(&environment)) + + // configure the knative trait + knTrait, _ := newKnativeTrait().(*knativeTrait) + ok, condition, err = knTrait.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the knative trait + require.NoError(t, knTrait.Apply(&environment)) + + // call the post step processors + for _, processor := range environment.PostStepProcessors { + assert.Nil(t, processor(&environment)) + } + + assert.True(t, *knTrait.Enabled) + assert.Contains(t, environment.Integration.Status.Capabilities, v1.CapabilityKnative) + assert.Contains(t, environment.Integration.Status.Dependencies, "mvn:org.apache.camel.k:camel-k-knative-impl") Review Comment: done -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537548157 ## pkg/trait/knative_test.go: ## @@ -348,6 +348,155 @@ func TestKnativePlatformHttpDependencies(t *testing.T) { } } +func TestKnativeEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('knative:channel/channel-source-1')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, + Platform: &v1.IntegrationPlatform{ + Spec: v1.IntegrationPlatformSpec{ + Cluster: v1.IntegrationPlatformClusterOpenShift, + Build: v1.IntegrationPlatformBuildSpec{ + PublishStrategy: v1.IntegrationPlatformBuildPublishStrategyS2I, + Registry: v1.RegistrySpec{Address: "registry"}, + }, + Profile: v1.TraitProfileKnative, + }, + }, + EnvVars:make([]corev1.EnvVar, 0), + ExecutedTraits: make([]Trait, 0), + Resources: k8sutils.NewCollection(), + } + environment.Platform.ResyncStatusFullConfig() + + // configure the init trait + init := NewInitTrait() + ok, condition, err := init.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the init trait + require.NoError(t, init.Apply(&environment)) + + // configure the knative trait + knTrait, _ := newKnativeTrait().(*knativeTrait) + ok, condition, err = knTrait.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the knative trait + require.NoError(t, knTrait.Apply(&environment)) + + // call the post step processors + for _, processor := range environment.PostStepProcessors { + assert.Nil(t, processor(&environment)) + } + + assert.True(t, *knTrait.Enabled) + assert.Contains(t, environment.Integration.Status.Capabilities, v1.CapabilityKnative) + assert.Contains(t, environment.Integration.Status.Dependencies, "mvn:org.apache.camel.k:camel-k-knative-impl") +} + +func TestKnativeNotEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('log:info')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, +
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
squakez commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1537232845 ## pkg/trait/knative_test.go: ## @@ -348,6 +348,155 @@ func TestKnativePlatformHttpDependencies(t *testing.T) { } } +func TestKnativeEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('knative:channel/channel-source-1')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, + Platform: &v1.IntegrationPlatform{ + Spec: v1.IntegrationPlatformSpec{ + Cluster: v1.IntegrationPlatformClusterOpenShift, + Build: v1.IntegrationPlatformBuildSpec{ + PublishStrategy: v1.IntegrationPlatformBuildPublishStrategyS2I, + Registry: v1.RegistrySpec{Address: "registry"}, + }, + Profile: v1.TraitProfileKnative, + }, + }, + EnvVars:make([]corev1.EnvVar, 0), + ExecutedTraits: make([]Trait, 0), + Resources: k8sutils.NewCollection(), + } + environment.Platform.ResyncStatusFullConfig() + + // configure the init trait + init := NewInitTrait() + ok, condition, err := init.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the init trait + require.NoError(t, init.Apply(&environment)) + + // configure the knative trait + knTrait, _ := newKnativeTrait().(*knativeTrait) + ok, condition, err = knTrait.Configure(&environment) + require.NoError(t, err) + assert.True(t, ok) + assert.Nil(t, condition) + + // apply the knative trait + require.NoError(t, knTrait.Apply(&environment)) + + // call the post step processors + for _, processor := range environment.PostStepProcessors { + assert.Nil(t, processor(&environment)) + } + + assert.True(t, *knTrait.Enabled) + assert.Contains(t, environment.Integration.Status.Capabilities, v1.CapabilityKnative) + assert.Contains(t, environment.Integration.Status.Dependencies, "mvn:org.apache.camel.k:camel-k-knative-impl") +} + +func TestKnativeNotEnabled(t *testing.T) { + catalog, err := camel.DefaultCatalog() + require.NoError(t, err) + + traitCatalog := NewCatalog(nil) + + environment := Environment{ + CamelCatalog: catalog, + Catalog: traitCatalog, + Integration: &v1.Integration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "ns", + }, + Status: v1.IntegrationStatus{ + Phase: v1.IntegrationPhaseInitialization, + }, + Spec: v1.IntegrationSpec{ + Profile: v1.TraitProfileKnative, + Sources: []v1.SourceSpec{ + { + DataSpec: v1.DataSpec{ + Name:"route.groovy", + Content: `from('timer:foo').to('log:info')`, + }, + Language: v1.LanguageGroovy, + }, + }, + }, + }, +
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1535770596 ## pkg/trait/knative.go: ## @@ -81,6 +83,24 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if err != nil { return false, nil, err } + + if t.Enabled == nil { Review Comment: I noticed it, but wanted to have a check before, as these filters are intended to check for the specifics of knative. Anyway, I changed the check to account for the results of the knative endpoints. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
claudio4j commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1535767234 ## pkg/trait/knative.go: ## @@ -70,7 +71,8 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if e.Integration == nil { return false, nil, nil } - if !pointer.BoolDeref(t.Enabled, true) { Review Comment: I noticed the `NewIntegrationConditionUserDisabled` says the trait is explicitly disabled, so this sets the condition only when the user sets `enabled=false`. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
squakez commented on code in PR #5275: URL: https://github.com/apache/camel-k/pull/5275#discussion_r1535621989 ## pkg/trait/knative.go: ## @@ -70,7 +71,8 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if e.Integration == nil { return false, nil, nil } - if !pointer.BoolDeref(t.Enabled, true) { Review Comment: Why do you need this? ## pkg/trait/knative.go: ## @@ -81,6 +83,24 @@ func (t *knativeTrait) Configure(e *Environment) (bool, *TraitCondition, error) if err != nil { return false, nil, err } + + if t.Enabled == nil { Review Comment: I think this is what the different `filterMetaItems` calls after this piece of code do. At the end of the `configure` you should have all the knative endpoints in the ChannelSources, ChannelSinks, ... variable. I think it is not convenient calculating this twice. Maybe, you can check those resources after they have been collected and do the needed logic accordingly. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Only enable knative trait when there is a knative endpoint [camel-k]
github-actions[bot] commented on PR #5275: URL: https://github.com/apache/camel-k/pull/5275#issuecomment-2015063857 :heavy_check_mark: Unit test coverage report - coverage increased from 37.2% to 37.3% (**+0.1%**) -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org