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

GAE Generator Enhancements and Corner Cases #10331

Closed
1 task done
SudharakaP opened this issue Sep 1, 2019 · 17 comments · Fixed by #10336
Closed
1 task done

GAE Generator Enhancements and Corner Cases #10331

SudharakaP opened this issue Sep 1, 2019 · 17 comments · Fixed by #10336

Comments

@SudharakaP
Copy link
Member

SudharakaP commented Sep 1, 2019

Overview of the feature request

This is a continuation of #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)

  1. 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

  • Checking this box is mandatory (this is just to show you read everything)
@SudharakaP
Copy link
Member Author

SudharakaP commented Sep 2, 2019

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
Copy link
Member

/cc @ludoch

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

@SudharakaP
Copy link
Member Author

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
Copy link
Member Author

SudharakaP commented Sep 4, 2019

@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
Copy link
Member

+1

@SudharakaP
Copy link
Member Author

@saturnism : Done 👍

@saturnism
Copy link
Member

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
Copy link
Member

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

@SudharakaP
Copy link
Member Author

SudharakaP commented Sep 5, 2019

@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
Copy link
Contributor

ludoch commented Sep 5, 2019

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
Copy link
Contributor

ludoch commented Sep 5, 2019

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
Copy link
Member Author

@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
Copy link
Member Author

SudharakaP commented Sep 5, 2019

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
Copy link
Member

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

@ludoch
Copy link
Contributor

ludoch commented Sep 5, 2019 via email

@SudharakaP
Copy link
Member Author

SudharakaP commented Sep 5, 2019

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
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants