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 persistence.xml to the war files #293

Merged
merged 3 commits into from Oct 7, 2019

Conversation

@mindhog
Copy link
Member

commented Sep 30, 2019

This fixes the App Engine build. The runtime now needs access to this.


This change is Reviewable

This fixes the App Engine build.  The runtime now needs access to this.
@mindhog mindhog requested a review from jianglai Sep 30, 2019
@googlebot googlebot added the cla: yes label Sep 30, 2019
@jianglai jianglai self-requested a review Sep 30, 2019
Copy link
Member

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jianglai and @mindhog)


appengine_war.gradle, line 42 at r1 (raw file):

    from("${coreResourcesDir}/META-INF/persistence.xml") {
        include "*.html"
    }

I think it should be from .../META-INF/, include persistence.xml? i. e. include the xml file from the folder.
Also, do we know where does the xml file end up? We'd want it to be in the right place in the WAR folder.

Copy link
Member

left a comment

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mindhog)

@@ -38,6 +38,10 @@ war {
from("${coreResourcesDir}/google/registry/ui/html") {
include "*.html"
}

from("${coreResourcesDir}/META-INF/persistence.xml") {
include "*.html"

This comment has been minimized.

Copy link
@gbrodman

gbrodman Oct 1, 2019

Collaborator

I think the .html part might be a remnant of a copy+paste?

Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman and @mindhog)


appengine_war.gradle, line 43 at r1 (raw file):

Previously, gbrodman wrote…

I think the .html part might be a remnant of a copy+paste?

Yeah, five minutes before you leave is never a good time to send a PR :-) Fixes coming as soon as I can safely test them...

This is the location prescribed both generally for war files and specifically
for app engine.

However, this still doesn't fix the release :-(
Copy link
Member Author

left a comment

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @gbrodman and @jianglai)


appengine_war.gradle, line 42 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…
    from("${coreResourcesDir}/META-INF/persistence.xml") {
        include "*.html"
    }

I think it should be from .../META-INF/, include persistence.xml? i. e. include the xml file from the folder.
Also, do we know where does the xml file end up? We'd want it to be in the right place in the WAR folder.

The latest version fixes the copy and also uses what is supposed to be the correct target location, WEB-INF/classes/META-INF, as documented in multiple sites. However, it still doesn't fix the release.

Use the DJTM until we get all of the dependencies set up for all of the
environments.

This shouldn't affect any of the unit tests, these use the
JpaTransactionManagerRule to set up a local database and connection.
@jianglai jianglai requested a review from gbrodman Oct 4, 2019
Copy link
Member

left a comment

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/model/transaction/TransactionManagerFactory.java, line 30 at r2 (raw file):

shicong)

I like it that you are adding a TODO for another engineer :P

Shouldn't we add a reason why we made the change here?

Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/model/transaction/TransactionManagerFactory.java, line 30 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…
shicong)

I like it that you are adding a TODO for another engineer :P

Shouldn't we add a reason why we made the change here?

:-) I figured he was more in the path of the dependencies. I had planned to communicate the details to him out of band, do you think that there should be more explanation in the code itself?

Copy link
Member Author

left a comment

I should note that with these changes (particularly the last one) this PR now runs in crash.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)

@jianglai

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)

core/src/main/java/google/registry/model/transaction/TransactionManagerFactory.java, line 30 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…
:-) I figured he was more in the path of the dependencies. I had planned to communicate the details to him out of band, do you think that there should be more explanation in the code itself?

I mean if we are going to keep this code for a while (and it looks like we will), it think it is better to document the reasons in the code.

@mindhog mindhog merged commit ebc8d54 into google:master Oct 7, 2019
4 of 7 checks passed
4 of 7 checks passed
code-review/reviewable 1 discussion left (gbrodman)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@mindhog mindhog deleted the mindhog:fix-release branch Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.