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

Integrate JetBrains google-app-engine-plugin into Google's plugin #780

Merged
merged 269 commits into from
Jul 15, 2016

Conversation

etanshaul
Copy link
Contributor

@etanshaul etanshaul commented Jul 12, 2016

Fixes #767

Base set of functionality needed to integrate the old plugin into ours to get everything running side by side with passing tests and passing static analysis checks.

More info coming..

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@etanshaul
Copy link
Contributor Author

etanshaul commented Jul 12, 2016

@chanseokoh @joaoandremartins @aslo

This PR merges the JetBrains GAE plugin into ours. It brings the JetBrains source into our module hierarchy while preserving the git history and attribution.

I made the minimal set of changes possible to get all the functionality working side by side (removing conflicts, making modifications where necessary etc.). All of the features provided by the old plugin are integrated into the google-cloud-tools-plugin module or a submodule of it.

When a user is running Ultimate Edition, then extra functionality is loaded (provided by the "ultimate" plugin).

There currently will be conflicts if running this project in IJ with the old JB plugin enabled. We will handle this in #768. For testing this out its best to manually disable the JB plugin:

image

There are also several other changes that are needed but can be separated into other PRs - deprecation of all usages of the old appcfg sdk, updating local run to use our common lib, etc.

I know this PR is pretty massive and tough to review so we can discuss in person if needed.

(Note about the branches: I created a new branch called integrate-jetbrains to which all PRs related to this effort will be merged for now. I wanted to keep master separate for any releases we may need to make prior to merging this all in).

@etanshaul etanshaul modified the milestone: 1.0-beta Jul 12, 2016
@etanshaul
Copy link
Contributor Author

As a guide, you can run through the JetBrains tutorial for their plugin and see that it still works with their plugin disabled (since the functionality has been merged in - App Engine standard project wizard, local run/debug, code whitelisting..) - excludes deploy since we have our own separate implementation:

https://www.jetbrains.com/help/idea/2016.1/getting-started-with-google-app-engine.html

@chanseokoh
Copy link
Contributor

Thanks for the link to the tutorial. I'll do some testing.

BTW, the build seems failing.

@etanshaul
Copy link
Contributor Author

Yea for some reason some tests are failing on Travis, I'll investigate
tomorrow.

On Tuesday, July 12, 2016, Chanseok Oh [email protected] wrote:

Thanks for the link to the tutorial. I'll do some testing.

BTW, the build seems failing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#780 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABp8QHHSfcy1PLwidv0COz5q7943f-DYks5qVA0HgaJpZM4JKztW
.

@etanshaul
Copy link
Contributor Author

I figured out the test failure problem: some new tests rely on .iml files which were being ignored by .gitignore so I added an exclusion for test data. It should pass now hopefully.

@chanseokoh
Copy link
Contributor

I noticed that New Project Wizard doesn't really work in CE. (UE seems fine.)

  • There is no web directory and WEB-INF is under root. (In UE, it's under web.)
  • index.jsp doesn't exist. (In UE, it's under web.)
  • A local run configuration is not setup automatically. In fact, CE can't create a new one, so there's no way to run a local dev server.

@etanshaul
Copy link
Contributor Author

Yes you are right. This is currently by design. The features you reference rely on UE functionality.

For the local dev server - I would have thought this would work in CE, but since support for AE standard apps rely on UE features this may not be the case. I'll look into this a little more.

@etanshaul
Copy link
Contributor Author

@chanseokoh - I looked into running the local dev server and this also relies on UE. It integrates into IJ's server functionality which is not available in CE. So everything you saw from your last comment is expected.

The only thing that is strange is it still lets you create a an app engine standard app from the wizard using Community Edition. We may want to disallow this later and instead only allow creation of a flex template (in a future PR).

@chanseokoh
Copy link
Contributor

Yeah, disallowing the wizard for standard seems like a good idea for now. (Or, at least adding index.jsp would be necessary, but not being able to run the local server is confusing and not consistent with the tutorial.)

@chanseokoh
Copy link
Contributor

I have played around a bit with creating a new project and running the local dev app server. They are working fine, so that's cool.

We should figure out what we will do with the licence header?

@etanshaul
Copy link
Contributor Author

Thanks for checking this out.

Do you mean the Jetbrains headers? That's intentional - we plan to keep their attribution (we also have the full git history).

@chanseokoh
Copy link
Contributor

Ah, yeah. Then it looks good. It is certainly unreasonable to review all of the JetBrains code, so I think we should move on and merge in the code after @joaoandremartins and @aslo also check that basic things are working, shouldn't we?

@etanshaul
Copy link
Contributor Author

Yes, agree.

@aslo
Copy link
Contributor

aslo commented Jul 14, 2016

I'm currently going through the sample tutorial and some other testing, will let you know if I find anything that needs to be addressed.

@etanshaul
Copy link
Contributor Author

That's great. Thanks.

@aslo
Copy link
Contributor

aslo commented Jul 14, 2016

I'm experiencing some issues with App Engine deployment. I created issue #786 with some more detail. I'll continue digging into the issue in the meantime.

@aslo
Copy link
Contributor

aslo commented Jul 14, 2016

Ok, I'm satisfied - as far as I can tell this looks good!

@etanshaul etanshaul force-pushed the merge branch 4 times, most recently from 2c224e6 to 96b557d Compare July 14, 2016 21:14
develar and others added 25 commits July 14, 2016 17:25
…cause it makes no sense to enter password and don't remember it
move modifiable models provider from maven to external system API for reuse
…EA Ultimate to allow creating new app engine projects in CE
…SDK' for modules which will remain on Java 6 and older
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.