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.

Sign up for GitHub

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

Jump to bottom

fix(#5097): Remove secondary IntegrationPlatform in favor of using IntegrationProfile #5138

Merged
merged 1 commit into from
Feb 28, 2024
Merged

fix(#5097): Remove secondary IntegrationPlatform in favor of using IntegrationProfile #5138

merged 1 commit into from
Feb 28, 2024

Conversation

christophd
Copy link
Contributor

  • Remove secondary IntegrationPlatform mode
  • Reduce logic to a single IntegrationPlatform per operator instance
  • Introduce IntegrationProfile custom resource definition
  • Let user customize a subset of IntegrationPlatform settings in IntegrationProfile
  • Load IntegrationProfile settings when integration resource is annotated to select the profile
  • Remove platform creation as part of the platform trait (avoids duplicate platform resources)
  • Save trait configuration used to build the integration kit on the resource spec for future reference

Release Note

fix(#5097): Remove secondary IntegrationPlatform in favor of using IntegrationProfile

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024

⚠️ Unit test coverage report - coverage decreased from 35.6% to 35.1% (-0.5%)

@christophd christophd force-pushed the issue/5097/integration-profile branch from 1e77a9b to eac72eb Compare February 5, 2024 19:43
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024

⚠️ Unit test coverage report - coverage decreased from 35.6% to 35.1% (-0.5%)

squakez
squakez reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through the details of the PR, but some early feedback for your consideration:

  1. we can't remove/rename any field from custom resource for backward compatibility reason. Anything we want to remove we must deprecate the fields and eventually remove them in a major upgrade. Same for the feature itself. We should deprecate it before removing to give the user the possibility to upgrade accordingly.
  2. we should avoid decreasing the coverage that much. Hopefully you can include some unit test to raise the coverage to be at least even.

@christophd
Copy link
Contributor Author

  1. we can't remove/rename any field from custom resource for backward compatibility reason. Anything we want to remove we must deprecate the fields and eventually remove them in a major upgrade. Same for the feature itself. We should deprecate it before removing to give the user the possibility to upgrade accordingly.

Regarding the CRD changes: do you refer to the removed trait options on the platform trait? What about auto migrating local IntegrationPlatform CRs to IntegrationProfile CRs?

  1. we should avoid decreasing the coverage that much. Hopefully you can include some unit test to raise the coverage to be at least even.

I just need to remove more code 😄 Yes, my plan was to add more unit tests

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.3% (-0.5%)

@christophd christophd force-pushed the issue/5097/integration-profile branch 2 times, most recently from 007363b to c15545e Compare February 20, 2024 20:11
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.3% (-0.5%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.5% (-0.3%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.5% (-0.3%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.5% (-0.3%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.5% (-0.3%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.7% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.7% (-0.1%)

@christophd
Copy link
Contributor Author

@squakez @lburgazzoli @gansheer @claudio4j the PR is now ready for review. If you want to have another look 🙏

I'd suggest to follow-up with these tasks in separate PRs:

  • Further simplification of integration platform logic with deprecate/remove namespace local integration platforms (strictly have only 1-1 relationship of operator and integration platform)
  • Restrict the settings that a user is able to set in integration profile and builder traits (admin related settings only settable via integration platform)

@lburgazzoli
Copy link
Contributor

@valdar @rinaldodev mind having a loom too ?

lburgazzoli
lburgazzoli reviewed Feb 27, 2024
View reviewed changes
pkg/apis/camel/v1/integrationprofile_types.go
)

// IntegrationProfileCondition describes the state of a resource at a certain point.
type IntegrationProfileCondition struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I know we have done this for may of the types but wonder if we can just reuse metav1.Condition, which would allow to manipulate conditions using some k8s utilities. The downside is that we won't have the specific condition type, but given 99% of the time is just an alias to strings and there are no real enums in go, I don't see anymore a very string benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the idea. I have created an issue for this #5191

gansheer
gansheer approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@gansheer gansheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Might be a good idea to regenerate the CRDs.

@lburgazzoli
Copy link
Contributor

Regarding the CRD changes: do you refer to the removed trait options on the platform trait? What about auto migrating local IntegrationPlatform CRs to IntegrationProfile CRs?

I think we should first translate the local IntegrationPlatform to a IntegrationProfile without removing it as this may have impacts to CI/CD pipelines, similar to what has been done for the transition from KameletBinding to Pipe

@christophd
Copy link
Contributor Author

I think we should first translate the local IntegrationPlatform to a IntegrationProfile without removing it as this may have impacts to CI/CD pipelines, similar to what has been done for the transition from KameletBinding to Pipe

The local IntegrationPlatform should still be a valid option after the changes in this PR

@lburgazzoli
Copy link
Contributor

I think we should first translate the local IntegrationPlatform to a IntegrationProfile without removing it as this may have impacts to CI/CD pipelines, similar to what has been done for the transition from KameletBinding to Pipe

The local IntegrationPlatform should still be a valid option after the changes in this PR

bet gets translated to an IntegrationProfile ?

@christophd
Copy link
Contributor Author

bet gets translated to an IntegrationProfile ?

no, the resources can coexist at the moment

@lburgazzoli
Copy link
Contributor

bet gets translated to an IntegrationProfile ?

no, the resources can coexist at the moment

do you think we can do the translation ? just to simplify the logic on the operator

@christophd
Copy link
Contributor Author

do you think we can do the translation ? just to simplify the logic on the operator

The resources are not exactly the same. The IntegrationProfile has only a subset of settings available on the IntegrationPlatform.

Do you have some auto translation in mind? or that the user needs to manually switch to using IntegrationProfile instead of IntegrationPlatform? I think the latter one is doable with some documentation. If we really want to simplify the logic on the operator regarding IntegrationPlatform we should remove the local platform feature and tell users to switch to IntegrationProfile

@lburgazzoli
Copy link
Contributor

do you think we can do the translation ? just to simplify the logic on the operator

The resources are not exactly the same. The IntegrationProfile has only a subset of settings available on the IntegrationPlatform.

Do you have some auto translation in mind? or that the user needs to manually switch to using IntegrationProfile instead of IntegrationPlatform? I think the latter one is doable with some documentation. If we really want to simplify the logic on the operator regarding IntegrationPlatform we should remove the local platform feature and tell users to switch to IntegrationProfile

No I don't so I'm fine with the current logic for the time being (which could be improved once we get #5119 merged).

I was mainly looking if there were a way to preserve backward compatibility in term of CRD/CR but simplify things internally.

@christophd
Copy link
Contributor Author

LGTM. Might be a good idea to regenerate the CRDs.

@gansheer I did a make generate and make bundle - am I missing something else?

@christophd
fix(apache#5097): Remove secondary IntegrationPlatform in favor of us…
ee54e63 
…ing IntegrationProfile

- Remove secondary IntegrationPlatform mode
- Reduce logic to a single IntegrationPlatform per operator instance
- IntegrationPlatforms matching the operatorID do have precedence when searching for local platforms
- Introduce IntegrationProfile custom resource definition
- Let user customize a subset of IntegrationPlatform settings in IntegrationProfile
- Load IntegrationProfile settings when integration resource is annotated to select the profile
- Save trait configuration used to build the integration kit on the resource spec for future reference
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 36.2% to 36% (-0.2%)

@christophd christophd merged commit cf6077c into apache:main Feb 28, 2024
16 checks passed
@lburgazzoli
Copy link
Contributor

@valdar @rinaldodev please have a look at this may impact #5119

@christophd christophd mentioned this pull request Mar 8, 2024
Closed
@squakez squakez mentioned this pull request Apr 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squakez squakez squakez left review comments

@lburgazzoli lburgazzoli lburgazzoli approved these changes

@oscerd oscerd oscerd approved these changes

@gansheer gansheer gansheer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@christophd @lburgazzoli @oscerd @gansheer @squakez