jbide-26175: cannot create server adapter for eap71-basic-s2i template #1785

bdshadow · 2018-07-31T14:25:55Z

Signed-off-by: Dmitrii Bocharov dbocharo@redhat.com

A very easy and little fix, but needs to be tested a lot, as can have some unpredictable repercussions.

jeffmaury · 2018-07-31T14:57:21Z

...src/org/jboss/tools/openshift/core/server/behavior/eap/OpenShiftEapDeployPathController.java

@@ -36,7 +36,7 @@ protected String getOutputNameFromSettings(IServerAttributes server, IModule mod
 			String ret = super.getOutputNameFromSettings(server, module);
 			if (ret == null && module.equals(findProjectModule(server))) {
 				String suffix = ServerModelUtilities.getDefaultSuffixForModule(module);
-				ret = "ROOT" + suffix;
+				ret = module.getName() + suffix;


That would probably fix for EAP 7.1 but for 7.0 as the OpenShift profile is probably not set you will not get the proper module name. Can you test with EAP 7.0 ?

@jeffmaury you're right. It fails. But i think it must be part of https://issues.jboss.org/browse/JBIDE-22138?focusedCommentId=13608828&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13608828. @adietish wdyt?

@bdshadow Yes, since the reason for the module name not being handled correctly lies in JBIDE-22138 aka upstream. As long as upstream isn't fixed the decision whether we want to apply this fix boils down to whether we want eap-70 or eap-71 templates to fail. I'd opt for sticking to the status quo aka not applying this fix.
@jeffmaury @bdshadow thoughts?

jeffmaury · 2018-08-02T08:47:06Z

As J2EEDeployableFactory is a singleton can't we call the clearCache from our stuff before we generate the module name ?

adietish · 2018-08-02T09:55:48Z

@jeffmaury I get the idea and yes, I agree: even though very dirty we could try to work around the bug in JBIDE-22138 and manually clear the module factory cache(s).

jkopriva · 2018-08-06T08:27:04Z

I am also against applying this patch, we need to have more complex solution to have both(eap70 and eap71) templates working.
As @jeffmaury mentioned, with this patch eap71 is working, but eap70 template does not work.

bdshadow · 2018-08-06T12:59:32Z

@adietish @jeffmaury @jkopriva "openshift" profile is now activated when an openshift server adapter is created. Anyway it doesn't solve our problems. Please, take a look and test it. WDYT?

bdshadow · 2018-08-07T10:24:23Z

@adietish @jkopriva i've tested it several times from scratch with a new connection and eclipse wotkspace without kitchensink project yet. It works for me.
I see a test is failing + i'll re-test the reflection stuff with java 9, but could you please verify that it works for you?

adietish · 2018-08-07T12:13:32Z

...src/org/jboss/tools/openshift/core/server/behavior/eap/OpenShiftEapDeployPathController.java

+			return OpenShiftServerUtils.findProjectModule(getEclipseProject(server));
+		}
+
+		private IProject getEclipseProject(IServerAttributes server) {


any particular reason for extracting the call to the util method to it's own private method?

adietish · 2018-08-07T12:15:56Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

 		return model.saveServer(monitor);
 	}
+
+	@SuppressWarnings("restriction")
+    private void setOpenshiftMavenProfileActive(IProject newDeployProject)  throws CoreException  {


maybe we could make this method even more useful by refactoring it to something like activateMavenProfile(String profilename, IProject project)?

adietish · 2018-08-07T12:25:28Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

+
+	@SuppressWarnings("restriction")
+    private void setOpenshiftMavenProfileActive(IProject newDeployProject)  throws CoreException  {
+	    IFile pom = newDeployProject.getFile(IMavenConstants.POM_FILE_NAME);


the file may only be retrieved if the project has the maven nature, so the call could be easily moved to l1112 making it more logical, I think.

adietish · 2018-08-07T12:37:37Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

+            if (profiles.stream().anyMatch(p -> OPENSHIFT_MAVEN_PROFILE.equals(p.getId())) && !activeProfiles.contains(OPENSHIFT_MAVEN_PROFILE)) {
+                activeProfiles.add(OPENSHIFT_MAVEN_PROFILE);
+                profileManager.updateActiveProfiles(facade, activeProfiles, false, true, new NullProgressMonitor());
+            }


l1111-1122 could be refactored to a set of 3 methods which then would be reusable - if then moved to a common bundle :

ProfileData getActiveProfiles(IProject project)

boolean isActiveProfile(String profileId, IProject project) - optional, could be included in setActiveProfile

void setActiveProfile(String profileId)

Looks like a benefit if done so, agree?

nickboldt · 2018-08-07T23:05:30Z

testPR

adietish · 2018-08-07T23:34:07Z

@bdshadow You can rebase this PR in order to fix the jenkins errors. I bumped openshift (in #1788) in order to fix the baseline check errors that caused those.

jkopriva · 2018-08-08T07:34:31Z

@bdshadow I have retested it with last commit and I was able to create server adapter for eap70 and eap71 template/

bdshadow · 2018-08-08T09:57:42Z

@jkopriva thank you
@adietish i also tested it against https://github.com/redhat-helloworld-msa/helloworld-msa/blob/master/hello.adoc (second cmd approach) and it works fine.

Now i'm going to refactor my code according to the remarks, rebase it and test against java 9.

adietish · 2018-08-08T11:19:13Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

+	@SuppressWarnings("restriction")
+    private void setOpenshiftMavenProfileActive(IProject newDeployProject)  throws CoreException  {
+	    IFile pom = newDeployProject.getFile(IMavenConstants.POM_FILE_NAME);
+	    if (newDeployProject.hasNature(IMavenConstants.NATURE_ID) && pom != null) {                   


checking IFile "pom" against being null isn't enough I believe. IContainer#getFile always returns an instance, regardless whether it exists or not. I think that we should also check pom.isAccessible()

adietish · 2018-08-08T11:25:01Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

+            List<ProfileData> profiles = profileManager.getProfileDatas(facade, new NullProgressMonitor());
+            List<String> activeProfiles = profiles.stream()
+                    .filter(p -> ProfileState.Active.equals(p.getActivationState()))
+                    .map(p -> p.getId())


this lambda can be replaced by a method reference for improved readability: ProfileData::getId

adietish · 2018-08-08T15:25:43Z

....openshift.ui/src/org/jboss/tools/openshift/internal/ui/server/ServerSettingsWizardPage.java

+            IMavenProjectFacade facade = MavenPlugin.getMavenProjectRegistry().create(pom, true, new NullProgressMonitor());
+            List<ProfileData> profiles = profileManager.getProfileDatas(facade, new NullProgressMonitor());
+            List<String> activeProfiles = profiles.stream()
+                    .filter(p -> ProfileState.Active.equals(p.getActivationState()))


writing down what we discussed/found out:
with a ~/.m2/settings.xml file, that also has profiles, filtering only on activation states this will have all those activated:

One needs to also filter autoactive profiles:

.filter(p -> ProfileState.Active.equals(p.getActivationState()) && !p.isAutoActive())

This will produce the correct results:

adietish · 2018-08-08T15:30:20Z

Another thing that we should do:

deactive the openshift profile if one stops (or deletes the server adapter)
activate the openshift profile if one (re-)starts the adapter

I thus think that activating the profile should be done where the adapter gets started, not upon creation like it is now

adietish · 2018-08-08T16:31:19Z

extracted the maven profile to it's own class MavenProfile (which can be activate and eventually deactivated) in org.jboss.tools.openshift.core.
Further work required to have it activated upon adapter start and deactivated upon adapter stop.

adietish · 2018-08-09T16:08:27Z

@bdshadow rebased and added tests for MaveProfile

Signed-off-by: Dmitrii Bocharov <dbocharo@redhat.com>

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish · 2018-08-10T23:23:38Z

testPR

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

Signed-off-by: Dmitrii Bocharov <dbocharo@redhat.com>

bdshadow · 2018-08-13T10:18:00Z

Tested with java 9. Works fine

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish · 2018-08-13T22:42:22Z

@bdshadow When trying the eap70 basic template, I have the project switched to the correct openshift profile but still the kitchensink.war artifact deployed. Weird enough, I see the correct clearCache(IProject) method being called by our workaround and accordingly the properties in the module holding the correct ROOT property.
When then looking at the name that's being chosen, I saw that we're using the module name (and not the deploy name as I would think we should): https://github.com/jbosstools/jbosstools-openshift/pull/1785/files#diff-a349d83b5835c45fbf04dc863de9429cR39
Prior versions of the code clearly used the deploy name property of the module. Somehow they went missing (maybe when I rebased?).
I therefore put your code back in place: 1ffdac4

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

bdshadow · 2018-08-14T09:15:48Z

@adietish actually i removed the usage of property name. I debugged it for eap70 and eap71 and it was returning the same for both: module.getName() and property. Moreover, if null is returned, you can find in the parent class that this logic with the property retrieval is run

I don't know why it didn't work for you with eap70. I've tested it lots of times already, and once i also had such a problem, and i wrote to you about it. Then everything was fine. I have no idea why this happens sometimes

adietish · 2018-08-14T14:15:40Z

we agreed that deploying using the deploy name, rather the module name is the right way to do it.
But then we face the next issue:
When switching the deploy name, the name of the war changes. A new (exploded) war is created in the temporary server-adapter directory in Eclipse, but the old (exploded) war is not removed. This causes both wars to be deployed

Steps:

have app, workspace project and server adapter in place (create the app using the eap70-basic-s2i template)
make sure that the server-adapter temporary directory in Eclipse only has 1 exploded war (either ROOT.war or jboss-kitchensink.war - delete the other one) - temporary server-adapter directory in Eclipse is at <workspace>/.metadata/.plugins/org.jboss.ide.eclipse.as.core/<adapter-name>/deploy
switch the maven profile of the workspace project (if it has the openshift profile, uncheck it. If it has it, check it)

Result:

a new deployed war directory is created in the temporary server-adapter directory. The existing one stays in place, it is not removed. Therefore rsync will copy both (exploded) wars to OpenShift

@robstryker thoughts?

adietish · 2018-08-14T18:49:45Z

To solve the above we have to be able to identify that a new war is to be created, the old one should be removed. I currently don't see how we can achieve this.
@robstryker can please provide some pointers? We have the "PROP_DEPLOY_NAME" property changing in a module which causes a new war to be created in the local "deploy" directory. We now need to remove the old one and remove the .deploy marker so that it gets undeployed in the wildfly in the pod.

We could simply clean the local "deploy" directory (local sever-adapter storage) before it gets rsynced up to the OpenShift pod.
The problem with that is that this wont remove the existing deployment in the (wildfly on the) pod. Resulting is an additional deployment with the new war name that conflicts with the existing one (that was not removed).
To remove that one, we'd have to kill the .deployed marker (see https://docs.jboss.org/author/display/WFLY10/Application+deployment#Applicationdeployment-MarkerFiles) which we currently don't have locally. For this to work there are a few bits missing:

before publishing the module to the pod, we'd have to rsync the pod down to the local "deploy" directory (in order to get the .deployed marker)
ex. via the following:

public class OpenShiftEapPublishController extends OpenShiftPublishController implements ISubsystemController {

	@Override
	public int publishModule(int kind, int deltaKind, IModule[] module, IProgressMonitor monitor) throws CoreException {
+		rsyncDown(monitor);
		return super.publishModule(IServer.PUBLISH_CLEAN, deltaKind, module, monitor);
	}

+	private MultiStatus rsyncDown(IProgressMonitor monitor) throws CoreException {
+		final RSync rsync = OpenShiftServerUtils.createRSync(getServer(), monitor);
+		final File localDeploymentDirectory = new File(getDeploymentOptions().getDeploymentsRootFolder(true));
+		return rsync.syncPodsToDirectory(localDeploymentDirectory, ServerConsoleModel.getDefault().getConsoleWriter());
	}

then we'd have to remove the .deployed marker for the oudated war
and finally we'd have to use oc rsync --delete to make sure whatever file is deleted locally gets deleted in the pod.

bdshadow · 2018-08-15T09:35:03Z

by now it's better to wait for @robstryker as we can be missing smth and solution can be easier (or in the contrary we don't see smth complex). I discussed it with @adietish yesterday. It seems to me that he agreed, though he won't be available for the following 2 weeks too

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

rhdevelopers-ci · 2018-08-29T16:45:39Z

...tools.openshift.core/src/org/jboss/tools/openshift/core/server/OpenShiftServerBehaviour.java

+	 * Cache is not cleaned properly and we get an incorrect name of the module for deployment
+	 * https://issues.jboss.org/browse/JBIDE-22138#comment-13617731
+	 */
+	private void clearJSTcache(IProject project) throws Exception {


Define and throw a dedicated exception instead of using a generic one.

adietish · 2018-08-29T18:04:01Z

@bdshadow I added an additional commit which makes sure that modules get notified of the cleared module cache. Things now work for me:

publishing to an eap71-basic-s21 app -> publishes as kitchensink.war (as seen in rsync console output)
publishing to an eap70-basic-s21 app -> publishes as ROOT.war (as seen in rsync console output)
publishing to a hello app -> publishes as ROOT.war (as seen in rsnyc console output)
- hello can be deployed to openshift via "oc" in the cmd line as described here: https://github.com/redhat-helloworld-msa/helloworld-msa/blob/master/hello.adoc#option-2-deploy-project-via-oc-cli
- then import hello as maven project to your workspace
- then create a server adapter for it
  @bdshadow @jkopriva could you please test this PR and give it a +1/-1?

ps. manually switching the maven profile once the war has already been published will NOT remove the previous jar (see #1785 (comment)). But this won't harm the intended fix for https://issues.jboss.org/browse/JBIDE-26175 where the war stays constant. I'll file this to a new issue

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

rhdevelopers-ci · 2018-08-29T22:54:31Z

...t/src/org/jboss/tools/openshift/test/core/server/OpenShiftModuleDeploymentPrefsUtilTest.java

 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;

 import junit.framework.TestCase;

 @RunWith(MockitoJUnitRunner.class)
 public class OpenShiftModuleDeploymentPrefsUtilTest extends TestCase {


Add some tests to this class.

rhdevelopers-ci · 2018-08-29T22:54:32Z

SonarQube analysis reported 6 issues

1 blocker
5 major

Watch the comments in this conversation to review them.

jeffmaury

+1 Tested on Win10 with both EAP7.0 and EAP7.1 quickstarts

bdshadow requested review from adietish, robstryker, jeffmaury and jkopriva July 31, 2018 14:25

jeffmaury reviewed Jul 31, 2018

View reviewed changes

adietish reviewed Aug 7, 2018

View reviewed changes

adietish reviewed Aug 8, 2018

View reviewed changes

adietish force-pushed the jbide-26175 branch 2 times, most recently from 5e4401b to 9be188d Compare August 8, 2018 15:53

adietish force-pushed the jbide-26175 branch 4 times, most recently from bae57fa to 44a104e Compare August 8, 2018 21:33

adietish force-pushed the jbide-26175 branch from 09b4dae to 7319444 Compare August 9, 2018 18:56

bdshadow added 2 commits August 9, 2018 20:58

jbide-26175: cannot create server adapter for eap71-basic-s2i template

5ff6ce5

Signed-off-by: Dmitrii Bocharov <dbocharo@redhat.com>

jbide-26175: fix test

7804ff3

Signed-off-by: Dmitrii Bocharov <dbocharo@redhat.com>

jbosstools deleted a comment from adietish Aug 10, 2018

[JBIDE-22138] fixed MavenProfileTest

8e52ef5

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish force-pushed the jbide-26175 branch from bcbdef9 to ab6a0cb Compare August 10, 2018 23:23

adietish force-pushed the jbide-26175 branch from ab6a0cb to 981e2c5 Compare August 11, 2018 22:15

[JBIDE-22138] added test for profile deactivation

45d55f5

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish force-pushed the jbide-26175 branch from 981e2c5 to 45d55f5 Compare August 11, 2018 22:21

jbide-26175: move JST cache clearance to the publish stage

5ba8f94

Signed-off-by: Dmitrii Bocharov <dbocharo@redhat.com>

[JBIDE-26175] corrected sonar complaints

3419b3d

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish force-pushed the jbide-26175 branch from 88d142b to 3419b3d Compare August 13, 2018 22:32

[JBIDE-26175] use deployname prop to determine file to deploy

1ffdac4

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

adietish force-pushed the jbide-26175 branch from 91a940a to 1ffdac4 Compare August 13, 2018 23:34

[JBIDE-26175] adding workaround that notifies modules of a cleared cache

a34ca5b

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

rhdevelopers-ci reviewed Aug 29, 2018

View reviewed changes

[JBIDE-26175] tested outputname when deployName/moduleName

e3b273a

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

rhdevelopers-ci reviewed Aug 29, 2018

View reviewed changes

adietish requested review from jeffmaury and removed request for robstryker August 30, 2018 08:35

jeffmaury approved these changes Aug 30, 2018

View reviewed changes

adietish removed the request for review from jkopriva August 30, 2018 16:15

adietish merged commit 0ab3fb7 into jbosstools:master Aug 30, 2018

adietish deleted the jbide-26175 branch August 30, 2018 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jbide-26175: cannot create server adapter for eap71-basic-s2i template #1785

jbide-26175: cannot create server adapter for eap71-basic-s2i template #1785

bdshadow commented Jul 31, 2018

jeffmaury Jul 31, 2018

bdshadow Jul 31, 2018

adietish Aug 2, 2018

jeffmaury commented Aug 2, 2018

adietish commented Aug 2, 2018

jkopriva commented Aug 6, 2018

bdshadow commented Aug 6, 2018

bdshadow commented Aug 7, 2018

adietish Aug 7, 2018

adietish Aug 7, 2018

adietish Aug 7, 2018

adietish Aug 7, 2018

nickboldt commented Aug 7, 2018

adietish commented Aug 7, 2018

jkopriva commented Aug 8, 2018

bdshadow commented Aug 8, 2018 •

edited

Loading

adietish Aug 8, 2018

adietish Aug 8, 2018

adietish Aug 8, 2018 •

edited

Loading

adietish commented Aug 8, 2018

adietish commented Aug 8, 2018

adietish commented Aug 9, 2018 •

edited

Loading

adietish commented Aug 10, 2018

bdshadow commented Aug 13, 2018

adietish commented Aug 13, 2018 •

edited

Loading

bdshadow commented Aug 14, 2018 •

edited by adietish

Loading

adietish commented Aug 14, 2018 •

edited

Loading

adietish commented Aug 14, 2018 •

edited

Loading

bdshadow commented Aug 15, 2018 •

edited

Loading

rhdevelopers-ci Aug 29, 2018

adietish commented Aug 29, 2018 •

edited

Loading

rhdevelopers-ci Aug 29, 2018

rhdevelopers-ci commented Aug 29, 2018

jeffmaury left a comment

jbide-26175: cannot create server adapter for eap71-basic-s2i template #1785

jbide-26175: cannot create server adapter for eap71-basic-s2i template #1785

Conversation

bdshadow commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffmaury commented Aug 2, 2018

adietish commented Aug 2, 2018

jkopriva commented Aug 6, 2018

bdshadow commented Aug 6, 2018

bdshadow commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickboldt commented Aug 7, 2018

adietish commented Aug 7, 2018

jkopriva commented Aug 8, 2018

bdshadow commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adietish Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

adietish commented Aug 8, 2018

adietish commented Aug 8, 2018

adietish commented Aug 9, 2018 • edited Loading

adietish commented Aug 10, 2018

bdshadow commented Aug 13, 2018

adietish commented Aug 13, 2018 • edited Loading

bdshadow commented Aug 14, 2018 • edited by adietish Loading

adietish commented Aug 14, 2018 • edited Loading

Steps:

Result:

adietish commented Aug 14, 2018 • edited Loading

bdshadow commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

adietish commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

rhdevelopers-ci commented Aug 29, 2018

jeffmaury left a comment

Choose a reason for hiding this comment

bdshadow commented Aug 8, 2018 •

edited

Loading

adietish Aug 8, 2018 •

edited

Loading

adietish commented Aug 9, 2018 •

edited

Loading

adietish commented Aug 13, 2018 •

edited

Loading

bdshadow commented Aug 14, 2018 •

edited by adietish

Loading

adietish commented Aug 14, 2018 •

edited

Loading

adietish commented Aug 14, 2018 •

edited

Loading

bdshadow commented Aug 15, 2018 •

edited

Loading

adietish commented Aug 29, 2018 •

edited

Loading