Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

Rearrage docs, add faq for thin jars #401

Merged
merged 4 commits into from
Sep 16, 2019
Merged

Rearrage docs, add faq for thin jars #401

merged 4 commits into from
Sep 16, 2019

Conversation

loosebazooka
Copy link
Contributor

@loosebazooka loosebazooka commented Sep 16, 2019

README.md Outdated
appengine:help|Displays help information on the plugin. Use `mvn appengine:help -Ddetail=true -Dgoal=[goal]` for detailed goal documentation.
Please see the [USER GUIDE](USER_GUIDE.md) for a full list of supported goals and configuration
options.
* [for app.yaml based projects](USER_GUIDE.md#app-engine-appengine-webxml-based-projects)
Copy link
Contributor

Choose a reason for hiding this comment

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

These two links are reversed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, good catch

USER_GUIDE.md Outdated
@@ -368,4 +368,41 @@ The plugin defaults to `appengine-web.xml` based deployment if your project cont
file. If your project also has an `src/main/appengine/app.yaml` file and you wish to use that, you may temporarily move the
`appengine-web.xml` file to a different location before deploying.

### How do I deploy an `app.yaml` based thin jar to appengine?
Copy link
Contributor

Choose a reason for hiding this comment

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

App Engine?

USER_GUIDE.md Outdated
@@ -368,4 +368,41 @@ The plugin defaults to `appengine-web.xml` based deployment if your project cont
file. If your project also has an `src/main/appengine/app.yaml` file and you wish to use that, you may temporarily move the
`appengine-web.xml` file to a different location before deploying.

### How do I deploy an `app.yaml` based thin jar to appengine?

You will probably need to stage the application to include it's dependencies. You might use something like the
Copy link
Contributor

Choose a reason for hiding this comment

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

it's its

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this whole thing, can you take another look?

README.md Outdated
@@ -9,29 +9,12 @@ This Maven plugin provides goals to build and deploy Google App Engine applicati
| :------------------------------ |
| 2.0.0 has been published. The behavior of the appengine-maven-plugin has changed since v1.+; please see the [CHANGELOG](CHANGELOG.md) for a full list of changes and an updated [USER GUIDE](USER_GUIDE.md) for details. If you are having trouble using or updating your plugin, please file a [new issue](https://github.com/GoogleCloudPlatform/app-maven-plugin/issues).|
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this section say 2.1.0 now, or is it fine to leave it as 2.0.0 since it's focused on v1 -> v2 migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can take this down and make it a section below. It's already been like 5 months since 2.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I'm just going to remove it and do nothing.

USER_GUIDE.md Outdated
</plugin>
```

Then when you run `mvn package appengine:deploy` the dependencies are copied and included as part of the deployment
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing period at the end

Copy link
Contributor

@TadCordle TadCordle left a comment

Choose a reason for hiding this comment

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

LGTM other than those two things

@loosebazooka loosebazooka merged commit 2a86b55 into master Sep 16, 2019

```
runtime: java11
entrypoint: java -Xmx64m -cp "*" com.example.MyMainClass
Copy link
Contributor

Choose a reason for hiding this comment

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

Is -Xmx64m recommended for App Engine? If not, remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, it was in the example on the appengine docs, so I used it.

</plugin>
```

Then when you run `mvn package appengine:deploy` the dependencies are copied and included as part of the deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong about this, but just in case:

I think a lot of frameworks nowadays will by default create a fat JAR, so maybe it'd be nice to explain a bit for those cases to deploy an app correctly. For example, Spring Boot will generate two JARs (fat one and the original one), and isn't it that this command will copy the fat JAR to the staging area (or both)?

BTW, there seems to be a working hack the user and @ludoch used to remove a fat JAR.

Copy link
Contributor Author

@loosebazooka loosebazooka Sep 16, 2019

Choose a reason for hiding this comment

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

Yes, I think I might lean towards thinking that that's on them. For springboot, in maven, I believe some of the configurations leads to it overriding the default final artifact as the springboot fat jar (but I could be wrong about the exact mechanism).

@chanseokoh chanseokoh deleted the rearrange-docs branch September 16, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants