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

Add flex JAR template #2545

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Add flex JAR template #2545

wants to merge 16 commits into from

Conversation

chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Oct 30, 2017

Towards #2470.

No UI, so this will be invisible to users.

The app created from this works when deployed.

@codecov
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

Merging #2545 into master will increase coverage by 0.16%.
The diff coverage is 85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2545      +/-   ##
============================================
+ Coverage     72.47%   72.63%   +0.16%     
- Complexity     2367     2381      +14     
============================================
  Files           347      348       +1     
  Lines         13837    13911      +74     
  Branches       1388     1389       +1     
============================================
+ Hits          10028    10104      +76     
- Misses         3237     3240       +3     
+ Partials        572      567       -5
Impacted Files Coverage Δ Complexity Δ
...com/google/cloud/tools/eclipse/util/Templates.java 80.64% <ø> (ø) 4 <0> (ø) ⬇️
...newproject/flex/CreateAppEngineFlexJarProject.java 0% <0%> (ø) 0 <0> (?)
...ls/eclipse/appengine/newproject/CodeTemplates.java 98.21% <100%> (+2.84%) 27 <6> (+7) ⬆️
.../flex/FlexMavenPackagedProjectStagingDelegate.java 81.81% <66.66%> (-2.5%) 12 <0> (ø)
...e/cloud/tools/eclipse/util/jobs/FuturisticJob.java 81.25% <0%> (+1.25%) 25% <0%> (+1%) ⬆️
...tools/eclipse/test/util/ThreadDumpingWatchdog.java 68.83% <0%> (+3.72%) 44% <0%> (+5%) ⬆️
...ibraries/LibraryClasspathContainerResolverJob.java 100% <0%> (+25%) 4% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938125b...521bccb. Read the comment docs.

@patflynn patflynn self-requested a review October 31, 2017 19:04
Copy link

@patflynn patflynn left a comment

Choose a reason for hiding this comment

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

HttpServer based is not the direction we want to go with this template. This should be based on Spring Boot. Feel free to ask me about this in person.

@elharo
Copy link
Contributor

elharo commented Oct 31, 2017

I agree that HttpServer based is not the ultimate goal we want to reach with this template. This is a step on the way and not yet user visible.

@patflynn
Copy link

Ah so the plan is to merge this and change it to be spring boot app before launch?

That's better, but I don't fully understand how this is an incremental step in that direction. Why not make it spring boot based from the start?

@chanseokoh
Copy link
Contributor Author

Fear not. I'll add a couple Spring Boot template files in this PR too.

@elharo
Copy link
Contributor

elharo commented Oct 31, 2017

I agree that HttpServer is an uncommon approach, and we should aim for something that's closer to what users are likely to want.

I'm still not convinced that SpringBoot is the way to go. Flex is far more general than that. I don't object to a SpringBoot wizard, but it shouldn't be the only option we offer. I don't want to give users the incorrect idea that Flex is about SpringBoot.

<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
<maven.compiler.showDeprecation>true</maven.compiler.showDeprecation>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to show deprecation?
In general I prefer to keep samples minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original and current standard env pom.xml had this, and this is currently in every pom.xml we generate. I'll remove all of them here. Just let me know if you change your mind.

@patflynn
Copy link

great. Thanks for adding the spring boot files.

Remove the HttpServer based project. It's a dead-end for users and adds to the maintenance burden.

@saturnism @lesv FYI if you guys want to validate the boot app.

@elharo
Copy link
Contributor

elharo commented Oct 31, 2017

I'm not sure about the HttpServer. It's not serving the same market as SpringBoot, but there are many apps for which it's exactly what's needed to run effectively on flex.

There are also many apps for which SpringBoot is clearly overkill. We don't want to lock people into it or suggest that SpringBoot is the blessed and only way to build a flex app.

I would like to see a simple Jetty based template too, though I'm not sure if we have resources to commit to it right now; and I certainly don't want to block this work waiting for something that can be added later.

@elharo elharo added the blocked label Nov 1, 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.

None yet

4 participants