Generator-jhipster: GAE Generator Enhancements and Corner Cases

[SudharakaP]

Overview of the feature request

This is a continuation of https://github.com/jhipster/generator-jhipster/issues/10196 which hopes to discuss and do some refactoring to the GAE App generator. It's my belief that this generator has some remaining issues outlined below.

1) If the user hasn't installed Google Cloud SDK our app generator prompts the user to do so but it also give the unnecessary option of overwriting the pom file. This should be avoided. Typical output looks like;

INFO! Using JHipster version installed globally
INFO! Executing jhipster:gae
INFO! Options: from-cli: true
Welcome to Google App Engine Generator (Beta)
WARNING! This sub-generator is still in development, please report bugs on Github
✖ You don't have the Cloud SDK (gcloud) installed.
Download it from https://cloud.google.com/sdk/install
/bin/sh: 1: gcloud: not found
✖ Unable to determine the default Google Cloud Project ID
conflict pom.xml
? Overwrite pom.xml? (ynaxdH)

2) I would suggest we remove part of the generator where it tries to install the App Engine Java SDK using gcloud components install app-engine-java --quiet. This has the potential to fail since the Cloud SDK installation can be done in several ways and it is operating system dependant. For example a typical case maybe in an Ubuntu or Debian where the user installs it using apt-get. Then he could encounter the following error;

https://askubuntu.com/questions/1146879/how-do-i-fix-the-google-cloud-sdk-component-manager-is-disabled-for-this-instal

I think the best way to handle this is to check whether app-engine-java component is installed and if not prompt the user to do so; same thing we do for Google Cloud SDK.

@saturnism @PierreBesson : If this is in agreement let me know. I'll create the PR. Sorry I mistakenly created this as a feature request. If you could please change it to a bug report instead.

Motivation for or Use Case

More user friendly GAE generator with less confusion.

Related issues or PR

10196 and #10285

• [x] Checking this box is mandatory (this is just to show you read everything)

[SudharakaP]

Thinking about 2) further an alternative approach is we could spit back the full error message along with a message saying that app-engine-java is "probably" not installed; which then the user could use to figure out the problem. In the current state app engine doesn't give the full error message;

INFO! Using JHipster version installed globally
INFO! Executing jhipster:gae
INFO! Options: from-cli: true
Welcome to Google App Engine Generator (Beta)
WARNING! This sub-generator is still in development, please report bugs on Github
Installing App Engine Java SDK
... Running: gcloud components install app-engine-java --quiet
INFO! Congratulations, JHipster execution is complete!

Let me know what you think is better. I vote for 2) so that some users it will auto install it and for other (hopefully few) there's some pointers available.

[saturnism]

/cc @ludoch

nice, for gcloud, i guess it should just fail immediately.

[SudharakaP]

Wonderful. Thanks for the confirmation @saturnism . I've created a PR which fixes both issues. Please let me know if you see any issues. 😄

[SudharakaP]

@saturnism , @PierreBesson : I was wondering adding one more thing to this; we probably should set the secure: always in app.yaml by default to redirect all http traffic to https since anyone who's using GAE will most probably use their managed certificates and this setting is something I think we can all forget about. 😄 If this is in agreement, I'll add this one as well.

[saturnism]

+1

[SudharakaP]

@saturnism : Done 👍

[saturnism]

While we are at it, could I trouble you to change the run commands that gets displayed at the end?

Run App Engine DevServer Locally: ./mvnw appengine:run -DskipTests
Deploy to App Engine: ./mvnw appengine:deploy -DskipTests -Pprod,prod-gae

package is now required first, so, ./mvnw package appengine:... -...`

We may need to do similar thing for Gradle build too.

[saturnism]

actually, a follow up would be fine. i feel this PR is ready to be merged as-is.

[SudharakaP]

@saturnism : It's a simple fix; I just did it in this pull request itself. For gradle I don't think we need to change that since even in the new version as per documentation ./gradlew appengineRun should work. 😄

[ludoch]

Hi,
Thanks for doing the work....
What I really would like to do as well is the take benefit of a exploded fatjar deployment for App Engine.
The Jib plugin does it automatically to help optimizing Docker layers.
GAE Java deployment also does it (since 2009), ie it will only deploy updated files from a previous deployment... When there is a single fatjar, all those upload optimizations are gone, even it the app changed 1 character!

Basically, if we could unjar the fatjar in the target/appengine-staging directory (and remove the orginal fatjar), the entrypoint would have to be like:

entrypoint: java -agentpath:/opt/cdbg/cdbg_java_agent.so=--log_dir=/var/log -noverify -XX:+AlwaysPreTouch -XX:TieredStopAtLevel=1 -Djava.security.egd=file:/dev/./urandom -cp BOOT-INF/resources/:BOOT-INF/classes/:BOOT-INF/lib/* com.mycompany.myapp.YOUAPPCLASSNAME

If this can be done, one extra goodies would be to configure App Engine to use a CDN to access the static resources instead of the JVM itself (from a fatjar which is also not optimal at all)...

To configure static resources in the app.yaml with an exploded jHipster fatjar, you would have to add this section in app.yaml:

handlers:

• url: (/.*)
  static_files: BOOT-INF/classes/static\1
  upload: __NOT_USED__
  require_matching_file: True
  login: optional
  secure: optional
• url: /.*
  script: unsused
• url: /.*/
  script: unused
  login: optional
  secure: optional
• url: .*
  script: unused
  login: optional
  secure: optional

With these 2 changes, we are back to the orginal Java GAE design from 2009 with appengine-werb.xml configuration: exploded war for optimal deployement and automatic static file servinf from a CDN. You would see the difference when loading an app where all the static html and images and js scripts are served immediately...

Finally, Instance classes of F1 do not work yet with jHipster, but this will be fixed for GA very soon.

Thanks again.
Ludo (who do not like fatjars at all:-)

[ludoch]

Also, I think for Java11 GAE runtime, gcloud components install app-engine-java is not needed at all. It it needed only for Java8 runtime.

[SudharakaP]

@ludoch : Thanks much for all the information and ideas. 👍

What I really would like to do as well is the take benefit of a exploded fatjar deployment for App Engine......

Just to be clear; I suppose what you mean by exploded fatjar is a thin jar correct?

Also, I think for Java11 GAE runtime, gcloud components install app-engine-java is not needed at all. It is needed only for Java8 runtime.

I have to check this one; if so we can remove the entire check for app-engine-java.

@saturnism @ludoch : I'll create two seperate PRs for both of these. 😄

[SudharakaP]

Also, I think for Java11 GAE runtime, gcloud components install app-engine-java is not needed at all. It it needed only for Java8 runtime.

@ludoch : Although in the documentation for Java 11 it says to use gcloud components install app-engine-java; https://cloud.google.com/appengine/docs/standard/java11/using-maven#setting_up_and_validating_your

[saturnism]

Can we table the significant thin jar changes for the next iteration? lets get the basics committed first :)

[ludoch]

Maybe a bug in the current Maven plugin?
Thanks for the note, I'll follow up...

On Thu, Sep 5, 2019, 4:16 PM Sudharaka Palamakumbura <
[email protected]> wrote:

Also, I think for Java11 GAE runtime, gcloud components install
app-engine-java is not needed at all. It it needed only for Java8 runtime.

@ludoch https://github.com/ludoch : Although in the documentation for
Java 11 it says to use gcloud components install app-engine-java;
https://cloud.google.com/appengine/docs/standard/java11/using-maven

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/10331?email_source=notifications&email_token=AAATRTOEZW4VCQB44C2MRK3QIGHMRA5CNFSM4ISYK43KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6BHCIA#issuecomment-528642336,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAATRTNACDQDF5BWNP454Z3QIGHMRANCNFSM4ISYK43A
.

[SudharakaP]

Maybe a bug in the current Maven plugin?
Thanks for the note, I'll follow up...

@ludoch : Not sure; Thanks much; of course let us know. 😄 I'll leave that for now until officially it's updated since IMHO why bother removing it and getting into trouble when the official documents says otherwise. 😄

@saturnism :

Can we table the significant thin jar changes for the next iteration? lets get the basics committed first :)

Surely, I was thinking about a separate PR for this one after the current PR is merged? Is that what you mean next iteration?

[saturnism]

Surely, I was thinking about a separate PR for this one after the current PR is merged? Is that what you mean next iteration?

yup!