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

Switching to the platform descriptor format generated with a new goal #200

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

aloubyansky
Copy link
Member

This PR is switching the platform to the new JSON descriptor format generated by a new Maven plugin goal.
The CI is expected to fail until we release Quarkus core version containing the goal.

@maxandersen
Copy link
Member

What's the new format ? Didn't we say we would make forward compatible changes to it or is the issue our readers don't allow new elements ?

@aloubyansky
Copy link
Member Author

You've actually seen it quite a few times and it was mentioned explicitly that it's a completely new format. It's not backward compatible and there is no point to adapt our legacy readers. The legacy platform descriptor API has been completely removed from the core.

@maxandersen
Copy link
Member

I thought the new format was for the registry with multiple new things not the individual platform.

Now I'm curious in more detail what changed as the old one was already quite generic.

@maxandersen
Copy link
Member

So - just so I grok the impact here - if users upgrade to new quarkus core it will fail if they don't use new quarkus platform either.

And likewise, impossible to downgrade core with a new platform ?

That seems quite a big breaking change imo or am I missing something here ?

@maxandersen
Copy link
Member

And if necessary its necessary I just haven't seen it mentioned and highlighted so we should make sure we notify/document the rationale giving against our general goal of not breaking things between versions abruptly.

@aloubyansky
Copy link
Member Author

So - just so I grok the impact here - if users upgrade to new quarkus core it will fail if they don't use new quarkus platform either.

AFAIK, the new format will appear in 1.13.0. Our tools work in the minor version range. So users upgrading from 1.12.x to 1.13.x will be fine.

And likewise, impossible to downgrade core with a new platform ?

AFAIR, we have never done that across minor versions. Changing micros aren't going to be affected by the change.

That seems quite a big breaking change imo or am I missing something here ?

That's right, you aren't missing anything in that regard.

@maxandersen
Copy link
Member

AFAIR, we have never done that across minor versions. Changing micros aren't going to be affected by the change.

just because we haven't in past doesn't mean we shouldn't consider the impact.

i.e. users can by accident end up in case where they have 0.11 platform and 0.13 core .something we before would handle (more) gracefully.

when/if kogito/camel or others have platforms that uses older versions we dont want to force breakage if can be avoided. i;ll look more into it tomorrow but on surface this sounds more like a 2.0 change than a 0.13 kind a change - but lets explore. hopefully im just seeing things you already considered and have ways to handle already :)

@maxandersen
Copy link
Member

the biggger related fix is quarkusio/quarkus#15413

@aloubyansky
Copy link
Member Author

AFAIR, we have never done that across minor versions. Changing micros aren't going to be affected by the change.

just because we haven't in past doesn't mean we shouldn't consider the impact.

Right. It wasn't a careless act. I was fully aware of the changes. There were good reasons too.

i.e. users can by accident end up in case where they have 0.11 platform and 0.13 core .something we before would handle (more) gracefully.

I don't think we ever supported this kind of mix. I think we are on the way to but not today, afaik.

when/if kogito/camel or others have platforms that uses older versions we dont want to force breakage if can be avoided.

Sure. If there are such plans today, I wasn't aware of those. Let me know.

i;ll look more into it tomorrow but on surface this sounds more like a 2.0 change than a 0.13 kind a change - but lets explore. hopefully im just seeing things you already considered and have ways to handle already :)

@maxandersen
Copy link
Member

maxandersen commented Mar 7, 2021

The issue I mention last week of platform for kogito product. Not a new concern (same happens if external user/company makes a platform) just that it is now becoming a reality so not just a "what if" scenario anymore.

@aloubyansky
Copy link
Member Author

You mentioned that too late, the PR has already been merged. My point was I didn't know any details about that before. So I couldn't take anything into account.

@maxandersen
Copy link
Member

Late ? We just need to consider what impact it will have and adjust accordingly.

@aloubyansky
Copy link
Member Author

Sure, we can always adjust. It's still late in terms of collecting requirements to take them into account implementing significant changes.

and removing extension categories inherited from the quarkus-bom
platform
@gsmet
Copy link
Member

gsmet commented Mar 18, 2021

So what should we do with this PR? From my point of view, we didn't support using an old platform with a new Quarkus so I would be in favor of merging it.

And if we want to support that now, we would support it from 1.13 and onward.

Does it make sense?

Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

LGTM - if spotted earlier i would have suggested we made a format that wouldnt break; i.e. not change bom from array to string but its not worth risking on now.

@aloubyansky
Copy link
Member Author

Thanks

@aloubyansky aloubyansky merged commit 734a89a into quarkusio:main Mar 19, 2021
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.

3 participants