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

Move spring-boot-starter dependency back to platform #320

Closed
ZheSun88 opened this issue Jun 19, 2018 · 21 comments
Closed

Move spring-boot-starter dependency back to platform #320

ZheSun88 opened this issue Jun 19, 2018 · 21 comments
Labels
Milestone

Comments

@ZheSun88
Copy link
Contributor

ZheSun88 commented Jun 19, 2018

This is basically the reverse change for the issue #298
remove the separate release configuration for spring-boot-starter(snapshot and publish),
revert POM setting.

@ZheSun88 ZheSun88 self-assigned this Jun 19, 2018
@pleku
Copy link
Contributor

pleku commented Jun 19, 2018

This was proposed by @mstahv as per recommended by @snicoll (?).

If we do this, eg. just release the vaadin-spring-boot-starter as an artifact together with the add-on and put both to the vaadin platform BOM, then the users will have to declare both the vaadin-spring-boot-starter and the vaadin-core as dependency to get everything in, like the component Java APIs. This is because the vaadin-spring-boot-starter does not anymore depend on the vaadin-core and thus it only depends on the add-on (and transitively on Flow, but not on components).

Or then we add all the component integrations as a dependency to the vaadin-spring-boot-starter, which makes it a bit weird IMO, like almost a "competing" thing to vaadin-core.

Please, how should it be @mstahv & @snicoll ?

@snicoll
Copy link
Contributor

snicoll commented Jun 19, 2018

This was proposed by @mstahv as per recommended by @snicoll (?).

The only thing that rings a bell is regarding the pluginManagement in the Vaadin bom. In Vaadin 8, there is dependency management for the starter and, IMO, you should keep that with 10 as removing it is a breaking change.

I am not sure I understand how that discussion is related to this particular issue though.

@pleku
Copy link
Contributor

pleku commented Jun 19, 2018

The only thing that rings a bell is regarding the pluginManagement in the Vaadin bom. In Vaadin 8, there is dependency management for the starter and, IMO, you should keep that with 10 as removing it is a breaking change.

Yes that is true, and that is what we had originally also for V10. But currently since in V10 the framework (flow) doesn't include the components nor have those as a dependency, those need to be declared separately. That is what the platform dependencies are for, eg. vaadin-core, it combines versions of Flow and the component integrations. See https://github.com/vaadin/platform/blob/master/vaadin-core/pom.xml

That is why we moved the vaadin-spring-boot-starter to be outside of the vaadin-bom and made it use it and have dependencies for vaadin-core and vaadin-spring-boot. The starter was following the platform versioning.

I am not sure I understand how that discussion is related to this particular issue though?

The issue is that if we move the starter to be back in the vaadin-bom, then you still have to declare vaadin-core as a dependency, and not just vaadin-spring-boot-starter to get all the things you need. Or we need to add eg. the component dependencies to the vaadin-spring-boot-starter.

@snicoll
Copy link
Contributor

snicoll commented Jun 19, 2018

Sorry, I still don't understand why adding the starter to the vaadin-bom means that the starter can't rely on vaadin-core. From a naming perspective it looks broken as I would definitely expect something called vaadin-bom to provide dependency management for vaadin-core.

If you decide to require a separate bom, then so be it but my general recommendation about this is as follows:

  • The purpose of the starter is to bring a reasonable set of dependencies for the getting started use case. If you believe that vaadin-core is part of this list (or has to be for backward compatible reason), then it must be provided by the starter
  • There was a BOM available that manages both the Vaadin framework versions and the Spring integration version (via the starter). I'd appreciate if you keep it that way. Having a bom for everything the starter manages but the starter feels broken to me. Again, that doesn't mean it has to be the same bom coordinates but I certainly appreciate if that was the case.

@pleku
Copy link
Contributor

pleku commented Jun 19, 2018

vaadin-bom to provide dependency management for vaadin-core.

It does so.

What makes it weird to me, sorry it might be just due to my limited knowledge, is that then we'd have to release the vaadin-spring-boot-starter with a dependency to the previous version of the vaadin-core, than what will get released in the next platform release, in which the starter will also be a part of.

In each platform release first the vaadin-bom, is released and then the vaadin-core which uses the released vaadin-bom.

Thus, when you declare a dependency to vaadin-spring-boot-starter and use the vaadin-bom eg, 10.0.1, I presume (but I'm not sure) that the BOM overrides the dependencies for the transitive dependency of vaadin-core, which will be for that particular vaadin-spring-boot-starter the previous version, eg. 10.0.0.

Oh how fun Maven is (sorry again)

@snicoll
Copy link
Contributor

snicoll commented Jun 19, 2018

In each platform release first the vaadin-bom, is released and then the vaadin-core which uses the released vaadin-bom.

Sorry to comment on a situation I don't fully understand but that looks broken to me. The very purpose of a BOM is to be a public contract for your users. If you release the bom first and then vaadin-core, does the bom refers to a version of vaadin-core that is not yet available?

The valid process for a BOM is to release the various components and at the very end the BOM that defines the various versions to use for a particular version of the bom (here the Vaadin version). An alternative is to have the BOM as part of the release process of the rest of those various components so that they are all released in one go.

@mstahv
Copy link
Member

mstahv commented Jun 20, 2018

Maybe the issue is that vaadin-core uses vaadin-bom (and vica versa). There is kind of a circular dependency. I guess we should change it so that vaadin-core should define the versions of its dependencies without the bom (maybe define versions as properties in parent -> can be defined just once for both core and bom). Just throwing ideas.

@pleku
Copy link
Contributor

pleku commented Jun 21, 2018

Yes, there is a circular dependency.

We've now added the starter to the platform BOM vaadin/platform@d28eee6

But the starter is still released separately from the platform, which is an issue as it should be inside the platform. We'll fix that by moving the starter from this repository to the platform.

@mstahv
Copy link
Member

mstahv commented Jun 21, 2018

👍 That probably makes it easier to do the release properly. Land this before attacking that: #323 (review)

@ZheSun88
Copy link
Contributor Author

ZheSun88 commented Jun 25, 2018

till now, i think this ticket can be closed.

@ZheSun88 ZheSun88 added this to the 10.0.0 milestone Jun 25, 2018
@mstahv
Copy link
Member

mstahv commented Jun 25, 2018

The starters still isn't moved to platform repository, is there a separate issue for that?

@ZheSun88
Copy link
Contributor Author

ZheSun88 commented Jun 25, 2018

I will reopen this issue.

@ZheSun88 ZheSun88 removed their assignment Jun 25, 2018
@ZheSun88 ZheSun88 removed this from the 10.0.0 milestone Jun 25, 2018
@ZheSun88 ZheSun88 reopened this Jun 25, 2018
@pleku
Copy link
Contributor

pleku commented Jun 25, 2018

There has been a mistake in the platform BOM, simply just a typo in the version property for the vaadin-spring-boot-starter, thus the platform BOM 10.0.0 declares a non-existing version of vaadin-spring-boot-starter.

There will soon be a new platform & vaadin-spring-boot-starter releases, version 10.0.1, which fixes that.

@ZheSun88 ZheSun88 added the bug label Jun 27, 2018
@pleku pleku added this to the 1.0 Maintenance milestone Jun 27, 2018
@denis-anisimov
Copy link

I don't understand what should be done in the end.
There is a long discussion but the final agreement is unclear.

@snicoll
Copy link
Contributor

snicoll commented Jul 26, 2018

Let me try to summarize my thoughts from a start.spring.io and general Maven perspective.

Vaadin 8 offers a vaadin-bom that provides dependency management for Vaadin artifacts. This includes the Spring support and the starter version. If a user imports this bom then Vaadin's opinions in term of dependency management are applied. They can therefore add the starter without bothering about the version. If they want to use an optional Vaadin artifact, they can add it without bothering about the version and the bom makes sure things are consistent.

This issue is about how Vaadin 10 broke that. I am asking to restore to this behaviour and, if not possible, discuss why that is and what options are available.

One important point in the discussion is "Vaadin artifacts". A "Vaadin" bom can't really have an opinion about third party dependencies (for instance SLF4J) unless Vaadin considers itself a "platform". If they do, that's fine but I'd argue that there is a need for what vaadin-bom used to be in 8.

Vaadin 10 has been removed from start.spring.io for this reason and I hope to see it re-added soon. Let me know if I can help in any way.

@denis-anisimov
Copy link

Sure.
This is a description of desired behavior.
But we should have here in the ticket final design decision how exactly it should be done.

And this is what my comment about: there should be a clear instructions what to do (and this is request for our team).
At the moment there is a lot of comments with various opinions/suggestions/thoughts but no final clear agreed solution how to solve this ticket. That's what I'm requesting from our team.

@pleku
Copy link
Contributor

pleku commented Aug 6, 2018

At the moment there is a lot of comments with various opinions/suggestions/thoughts but no final clear agreed solution how to solve this ticket. That's what I'm requesting from our team.

What is still remaining is:

But the starter is still released separately from the platform, which is an issue as it should be inside the platform. We'll fix that by moving the starter from this repository to the platform.

So thus the starter module should be moved and released in the platform, instead of here.

@denis-anisimov
Copy link

Should this ticket be moved into platform then ?

@pleku
Copy link
Contributor

pleku commented Aug 6, 2018

I don't think it matters that much, but sure.

@pleku
Copy link
Contributor

pleku commented Aug 6, 2018

Issue moved to vaadin/platform #257 via ZenHub

@denis-anisimov
Copy link

Well, I'm personally not familiar with agreements and the way of adding modules/release build of platform.
So I think it's better if this task is done by the platform team instead of us (me at least) since they know more.
Technically since we have Kirill he may do this on behalf of our team....

@ZheSun88 ZheSun88 modified the milestones: 1.0 Maintenance, Abandoned Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants