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.

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

WIP separate v1 and v1alpha1 api packages as much as possible #2951

Merged
merged 6 commits into from
Jan 31, 2022

Conversation

djencks
Copy link
Contributor

@djencks djencks commented Jan 27, 2022

(on top of #2899)

I think the relationship between camel-k (v1) and kamelets (v1alpha1) should be minimized. Currently there are 5 uses of v1 types in v1alpha1:

RawMessage. This is, IMO, silly, since there's a v1alpha1 RawMessage. Using it everywhere in v1alpha1 is easy and doesn't appear to cause any problems.

Template. This type is used only in v1alpha1 and causes no problems when moved there.

Flow. This is deprecated in favor of Template. I suggest we remove it or set a date to remove it. It appears to be easy to copy a flow to a template.

The other two may not be appropriate to copy to v1alpha1, I don't understand how this is used well enough to have an opinion:

IntegrationSpec is used in KameletBindingSpec, as an optional member. Does it have the same meaning here as when used in camel-k?

SourceSpec is used in KameletSpec. Does it have the same meaning here as when used in camel-k?

At the moment this PR deals with the first three uses:

  • only use v1alpha1.RawMessage in v1alpha1
  • move Template to v1alpha1
  • copy Flow, when found, to a Template.

Locally, running the make commands mentioned in releasing.adoc succeed and don't cause further changes. We'll see what happens with tests run from the PR....

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 think it's good not to have crossing APIs versions, I think I introduced that problem myself in my first days of development 😟
PR #2932 is taking care to remove the .Flow, so it's okey to let it out from this as it will be merged later.

@djencks
Copy link
Contributor Author

djencks commented Jan 28, 2022

I think what makes the most sense is for me to rebase this on top of your branch squakez:chore/2751_remove_deprecated, otherwise we'll be creating conflicts around the flow >> template translation. If you want to pull in the relevant changes to your branch that's fine with me also.

@squakez
Copy link
Contributor

squakez commented Jan 28, 2022

I think what makes the most sense is for me to rebase this on top of your branch squakez:chore/2751_remove_deprecated, otherwise we'll be creating conflicts around the flow >> template translation. If you want to pull in the relevant changes to your branch that's fine with me also.

Okey. I am still fine tuning that PR as that .Flow removal must be performed also on documentation and all Kamelets we have along (see apache/camel-kamelets#750)

@djencks
Copy link
Contributor Author

djencks commented Jan 30, 2022

I suspect it will be simplest if you pull my simplified PR squakez#9 into your branch before trying to rebase your branch on main.

@squakez
Copy link
Contributor

squakez commented Jan 31, 2022

I suspect it will be simplest if you pull my simplified PR squakez#9 into your branch before trying to rebase your branch on main.

Yes, you can proceed with this as I am evaluating the possibility to postpone removing .kamelet.spec.flow at all.

@djencks djencks merged commit 7adbc42 into apache:main Jan 31, 2022
@djencks djencks deleted the main-refactor-api branch January 31, 2022 18:29
@djencks
Copy link
Contributor Author

djencks commented Jan 31, 2022

I hope I interpreted your comment correctly as suggesting I merge this PR... if I misunderstood feel free to revert the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants