Skip to content

Conversation

@dogeared
Copy link
Contributor

@dogeared dogeared commented Nov 7, 2016

No description provided.

@snicoll
Copy link
Contributor

snicoll commented Nov 10, 2016

Thank you for the PR. I had a review of your proposal according to our guidelines for 3rd party starters and I have a few suggestions:

Given that you have more than one starters, it would be beneficial that you provide a Bills of Materials (BOM) for the dependencies that you are exposing (and only that). That way, users could start with the default starter and change it without having to specify a version. The BOM must only define dependencies that you provide yourself, it shouldn't provide dependency management for Spring Boot or any 3rd party library that you'd be using.

We prefer to restrict to one starter per library so we would be accepting the default one only.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 10, 2016
@snicoll
Copy link
Contributor

snicoll commented Nov 20, 2016

ping @dogeared

@dogeared
Copy link
Contributor Author

Thanks for the ping! I am working on this. I will update this week.

Micah Silverman, CSM http://bit.ly/WikiScrum
Phone: 631.606.8928

Co-Author: Mastering Enterprise Javabeans 3.0 http://bit.ly/MasteringEJB3

"Debugging is twice as hard as writing the code in the first
place. Therefore, if you write the code as cleverly as
possible, you are, by definition, not smart enough to debug

it." by Brian Kernighan

[image: Namez]
http://namez.com/profiles/1720-micah-silverman/?autoPlay=true

On Sun, Nov 20, 2016 at 6:35 PM, Stéphane Nicoll notifications@github.com
wrote:

ping @dogeared https://github.com/dogeared


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#307 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWQEgQUslKJfSI-B9jd-Lcft7cg9OCfks5rANlHgaJpZM4KrvnE
.

@dogeared
Copy link
Contributor Author

Hi @snicoll:

It's unlikely that a developer would be using more than one Stormpath Starter in a project and we release all the starters togethers. Since the BOM itself is versioned, I don't think there's a lot of value in setting up a BOM. I may be missing something, however.

In the meantime, I will update the PR with just the default starter. Thanks!

@dogeared
Copy link
Contributor Author

@snicoll - updated to have only the default starter

@snicoll
Copy link
Contributor

snicoll commented Nov 21, 2016

I am not sure I agree. Don't you have any additional (optional) component that can be added besides the starter? If so, users will have to specify the version themselves (rather than having that managed for them). If you have a case where two stormpath-specific dependencies can be added to the project, we'd prefer to have a BOM.

@snicoll
Copy link
Contributor

snicoll commented Dec 1, 2016

ping @dogeared

@dogeared
Copy link
Contributor Author

dogeared commented Dec 3, 2016 via email

@dogeared
Copy link
Contributor Author

dogeared commented Dec 22, 2016

Hi @snicoll - We are planning to update our Java SDK for BOM for our 1.3.0 release. We are expecting to have this deployed sometime next week. I'll update this PR then.

One question: the stormpath-default-spring-boot-starter encapsulates all of our other spring boot starters. While developers can mix & match our starters (for instance, they could use stormpath-webmvc-spring-boot-starter without using the stormpath-spring-security-webmvc-spring-boot-starter), the stormpath-zuul-spring-cloud-starter is a bit of a different beast. As its name implies, it's a Stormpath integration for Zuul. Would it be possible for us to include that as an option in the Cloud Routing section of initializr?

@dogeared
Copy link
Contributor Author

dogeared commented Dec 28, 2016

Hi @snicoll: I've updated the PR to reflect use of a BOM as well as referencing the latest version of Stormpath.

NOTE: Stormpath 1.3.0 is not released as of yet. We expect to release tomorrow or Friday. So, if the PR is acceptable, please do not merge just yet. I wanted to update it to make sure it met with your approval.

I did include a second reference in the Cloud Routing section to our Zuul integration. Being Zuul, it is very different from our other Spring Boot starter offerings. If there's a better approach that would make it easy for start.spring.io users to make use of the Stormpath default and zuul starters, please let me know.

You can see our bom definition here: https://github.com/stormpath/stormpath-sdk-java/blob/1.3.x/bom/pom.xml

Thanks for your time and attention on this.

@snicoll
Copy link
Contributor

snicoll commented Dec 29, 2016

I don't have an immediate answer for Cloud routing but i'll raise it internally and we'll see.

The bom looks alright; I have two remarks though:

  • Could you please hard-code the version for each artifact, rather than using project.version there. If I import that bom in my own project, project.version refers to the version of my project, not the one of stormpath. In practice, it won't have any impact as the version resolution happens locally but I think it's much more reasable (and correct) when the versions are explicit. These are replaced automatically at release time anyway
  • oss-parent is deprecated for quite a long time and it's highly unusual that a bom inherits from it. I understand why you did it but it would be much nicer if the bom did not provide that stuff. Again, it's not a blocker because it won't make a difference but newer maven versions may change that.

See here for more details of how you can move away from oss-parent.

@dogeared dogeared changed the title Make two of the primary Stormpath Spring integrations available on initializr Make the primary Stormpath Spring integration available on initializr Jan 11, 2017
@dogeared
Copy link
Contributor Author

Hi @snicoll - We are releasing 1.3.0 of our Stormpath Java SDK today. I've updated the PR here and I addressed your other comments regarding the BOM here: https://github.com/stormpath/stormpath-sdk-java/blob/master/bom/pom.xml. I'd appreciate a final look and I will let you know here when 1.3.0 is released so that you can merge, if approved. Thanks!

@snicoll
Copy link
Contributor

snicoll commented Jan 11, 2017

Thanks for reaching out!

This is not a blocker in any way, but I woudn't expose the stormpath.version in any way. If you import the stormpath-bom 1.3.0 I don't think it makes sense that you should be able to override the stormpath version (I am sure you know it doesn't work with bom imports anyway). If you want to change the version, you should change the version you use to import the bom. Same thing if you use the bom as a parent.

Also, let's assume that you add a new module in 1.4.0. You could extend from 1.3.0, override the version to 1.4.0 and get an incomplete bom because the new extra module isn't there.

We've had the same discussion around spring-boot.version and we've decided to remove it altogether.

I would just hardcode the version in every module there. Thanks!

@snicoll snicoll added type: enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2017
@snicoll snicoll self-assigned this Jan 12, 2017
snicoll pushed a commit to snicoll/initializr that referenced this pull request Jan 13, 2017
snicoll added a commit to snicoll/initializr that referenced this pull request Jan 13, 2017
@dogeared
Copy link
Contributor Author

Hello @snicoll - We've releases 1.5.0 of the Java SDK, which depends on the 1.5.0 release of Spring Boot. The bom no longer has any overrides in it: https://github.com/stormpath/stormpath-sdk-java/blob/stormpath-sdk-root-1.5.0/bom/pom.xml.

I've pushed an updated version of the PR with squashed commits. Assuming it meets with your approval, please merge after initializr has been updated to make spring boot 1.5.0 the default.

Thanks!

snicoll pushed a commit that referenced this pull request Jan 31, 2017
@snicoll snicoll closed this in 49bfaae Jan 31, 2017
snicoll added a commit that referenced this pull request Jan 31, 2017
* pr/307:
  Polish Stormpath entry
  Add Stormpath Default Spring Boot Starter
@snicoll snicoll added this to the 0.3.0 milestone Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants