search

cloudfoundry-attic / eclipse-integration-cloudfoundry

Cloud Foundry Integration for Eclipse
Apache License 2.0
41 stars 47 forks source link

Code refactor to provide Java Web Application Archive through an ApplicationDelegate and some code cleanup #42

Closed orlandoibm closed 9 years ago

orlandoibm commented 9 years ago

Proposed changes: 1) Making current JavaWebApplicationDelegate to provide a WAR for Java Web Applications instead just relying on the default fallback in StartOperation.performDeployment() (this helps on cleaning the implementation to make it consistent with all other ApplicationDelegate contributions). 2) Removed the case for war file directly passed as parameter to StartOperation.pushApplication(); this was useful for the case when the default fallback directly provided a File (warFile) variable, but now that we are moving to every case returning an ApplicationArchive, that path is just never exercised.

cfdreddbot commented 9 years ago

Hey orlandoibm!

Thanks for submitting this pull request!

All pull request authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions.

cf-gitbot commented 9 years ago

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/95914066.

nierajsingh commented 9 years ago

Thanks for refactoring the code so that ApplicationArchive is used consistently for both WAR and JAR.

We've made some changes in the same code base that affects your PR, particularly fixing a bug with the use of ZipFile that was causing a resource leak as well as some other issue with pushing a JAR.

If it would be possible for you to merge the latest changes into your PR and resubmit. Also one additional change in your PR related to this fix is to use the newly introduced CloudZipApplicationArchive:

https://github.com/cloudfoundry/eclipse-integration-cloudfoundry/blob/910e6cfbf7284c2251057c6dd63e447d9fbea0cd/org.cloudfoundry.ide.eclipse.server.core/src/org/cloudfoundry/ide/eclipse/server/core/internal/application/CloudZipApplicationArchive.java

Instead of ZipApplicationArchive in:

https://github.com/cloudfoundry/eclipse-integration-cloudfoundry/pull/42/files#diff-ed04eb6dc02d76b691d784aad8abf902R202

Otherwise the PR looks good and after you make the changes I'll merge the PR.

Thanks.

orlandoibm commented 9 years ago

Hello @nierajsingh. I have done the changes you suggested, please take a look at the current status of the pull request.

Thanks.

nierajsingh commented 9 years ago

Thanks for making the changes.

I'll merge the PR but we may make an additional change to your PR as part of another fix related to this area, where we remove using a default Java web application delegate as specified here:

https://github.com/cloudfoundry/eclipse-integration-cloudfoundry/pull/42/files#diff-b62df44162266b36be07eea7f180d272R150

The reason for this is that we have a default jst.web application delegate contributed through the application framework extension point:

https://github.com/cloudfoundry/eclipse-integration-cloudfoundry/blob/master/org.cloudfoundry.ide.eclipse.server.core/plugin.xml#L58

that always gets loaded for any module with jst.web, and therefore acts as a default Java web application provider.

However, as we need this merged into the code base for our other work, we'll merge this now, and make the change as part of our commit.

Thanks again for doing the work and submitting this PR.