[JBIDE-24184] get debug port and devmode switch from image #1518

adietish · 2017-06-19T11:15:58Z

Pull Request Checklist

General

Is this a blocking issue or new feature? If yes, QE needs to +1 this PR

Code

Are method-/class-/variable-names meaningful?
Are methods concise, not too long?
Are catch blocks catching precise Exceptions only (no catch all)?

Testing

Are there unit-tests?
Are there integration tests (or at least a jira to tackle these)?
Is the non-happy path working, too?
Are other parts that use the same component still working fine?

Function

Does it work?

adietish · 2017-07-04T20:38:26Z

@bdshadow @jeffmaury @rhopp could you please test this huge one?

the changes here are (among a general overhaul of the server adapter code):

allow users to provide env keys to enable/disable dev_mode, debug_port and the debug port value
use docker image provided values if the user chooses not to provide those
if devmode has to be enabled on top of debug_mode, do that in a single update

bdshadow

Andre, look at some of the remarks after a quick look. Of course i'll review and test it more later.

bdshadow · 2017-07-07T14:50:44Z

...ift.common.core/src/org/jboss/tools/openshift/internal/common/core/util/CollectionUtils.java

+	 * @param collection
+	 * @return
+	 */
+	public static <T> boolean isEmpty(Collection<T> collection) {


don't think that we really need it. Apache CollectionUtils has already this method and can be seen from all the places of usage of this isEmpty.

bdshadow · 2017-07-07T14:52:51Z

...s/tools/openshift/internal/common/ui/databinding/DisablableRequiredStringMultiValidator.java

 		}
 		return ValidationStatus.ok();
 	}

+	protected boolean isDisabled() {
+		Boolean disabled = this.disabledObservable.getValue();


return this.disabledObservable.getValue(); is enough here

i dont think so, since it could be null

bdshadow · 2017-07-07T14:55:09Z

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

+		DockerImageURI uri = trigger.getFrom();
+
+		return getImageStreamTag(uri, resource.getNamespace());
+//		String imageRef = getImageRef(dc, connection);


do we need such a big comment left here?

@adietish i think we can remove it

bdshadow · 2017-07-07T15:02:37Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

+			boolean running = POD_STATE_RUNNING.equalsIgnoreCase(it.next().getStatus());
+			if (expectedRunning == !running) {
+				return false;
+			} else if (expectedRunning == running) {


smth really strange is going here. If we go to else, there is no sense to check expectedRunning == running - it'll be 100% true. And why do we return false in both cases?

lol, indeed +100. this looks like a snippet worth publishing to "coding horror".

bdshadow · 2017-07-10T13:38:15Z

...nshift.test/src/org/jboss/tools/openshift/test/core/server/debug/OpenShiftDebugModeTest.java

+	}
+
+	@Test
+	public void testEnableDebug() throws CoreException {


possibly, worth deleting these comments too

this test is in the doing

bdshadow · 2017-07-10T22:19:19Z

@adietish tested multiple times with eap application. Can't debug it ;(

create eap70-basic-s2i application
create server adapter (based on pod or service - doesn't matter)
restart server adapter in debug mode
4 (optional to make sure server adapter deploy changes)) Try to change smth in java code and make sure the changes are applied
put breakpoint somewhere
try to execute smth in application so that the line with breakpoint would be used.
breakpoint was not

adietish · 2017-07-10T22:46:30Z

@bdshadow
I believe that I found the problem. Debugging is correctly initiated but an outdated pod is being used (when enabling debugging a new pod is launched, but the code is still referring to the terminated pod). I see this by the following:
In the Eclipse log I see the following exception (notice the pod name):

Caused by: org.eclipse.core.runtime.CoreException: Pod port specified in server adapter "eap-app (Service) at OpenShift 3 (192.168.64.2)" is no present in pod "eap-app-1-vtz7b"

while in the explorer, as child of the service, I see a different pod namely eap-app-1-qtnkb

rhdevelopers-ci · 2017-07-11T10:17:44Z

...boss.tools.openshift.core/src/org/jboss/tools/openshift/core/OpenShiftResourceSelectors.java

+ * 
+ * @author Andre Dietisheim
+ */
+public interface OpenShiftResourceSelectors {


Move constants to a class or enum.

rhdevelopers-ci · 2017-07-11T10:17:45Z

...penshift.core/src/org/jboss/tools/openshift/internal/core/docker/ImageStreamTagMetaData.java

@@ -27,24 +29,27 @@
 	private static final String ENV = "Env";
 	private static final String EXPOSED_PORT = "ExposedPorts";
 	private static final String VOLUMES = "Volumes";
+	private static final String LABELS = "Labels";

 	private final ModelNode node;
 	private final String [] CONFIG_ROOT;


Rename this field "CONFIG_ROOT" to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

rhdevelopers-ci · 2017-07-11T10:17:45Z

...penshift.core/src/org/jboss/tools/openshift/internal/core/docker/ImageStreamTagMetaData.java

@@ -27,24 +29,27 @@
 	private static final String ENV = "Env";
 	private static final String EXPOSED_PORT = "ExposedPorts";
 	private static final String VOLUMES = "Volumes";
+	private static final String LABELS = "Labels";

 	private final ModelNode node;
 	private final String [] CONFIG_ROOT;
 	private final String[] PORT_KEY;


Rename this field "PORT_KEY" to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

rhdevelopers-ci · 2017-07-11T10:17:45Z

...penshift.core/src/org/jboss/tools/openshift/internal/core/docker/ImageStreamTagMetaData.java

@@ -27,24 +29,27 @@
 	private static final String ENV = "Env";
 	private static final String EXPOSED_PORT = "ExposedPorts";
 	private static final String VOLUMES = "Volumes";
+	private static final String LABELS = "Labels";

 	private final ModelNode node;
 	private final String [] CONFIG_ROOT;
 	private final String[] PORT_KEY;
 	private final String[] VOLUMES_KEY;


Rename this field "VOLUMES_KEY" to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

rhdevelopers-ci · 2017-07-11T10:17:46Z

...penshift.core/src/org/jboss/tools/openshift/internal/core/docker/ImageStreamTagMetaData.java

@@ -27,24 +29,27 @@
 	private static final String ENV = "Env";
 	private static final String EXPOSED_PORT = "ExposedPorts";
 	private static final String VOLUMES = "Volumes";
+	private static final String LABELS = "Labels";

 	private final ModelNode node;
 	private final String [] CONFIG_ROOT;
 	private final String[] PORT_KEY;
 	private final String[] VOLUMES_KEY;
 	private final String[] ENV_KEY;


Rename this field "ENV_KEY" to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

adietish · 2017-08-09T08:55:31Z

@jeffmaury @bdshadow as discussed and agreed upon we should merge this asap when branch freeze is repealed

rhdevelopers-ci · 2017-08-10T00:23:03Z

...tools.openshift.core/src/org/jboss/tools/openshift/internal/core/util/NewPodDetectorJob.java

+		Connection connection = ConnectionsRegistryUtil.getConnectionFor(rc);
+		List<IPod> allPods = connection.getResources(ResourceKind.POD, rc.getNamespace());
+		return ResourceUtils.getPodsFor(rc, allPods).stream()
+			.filter(pod -> ResourceUtils.isRuntimePod(pod))


Rename "pod" which hides the field declared at line 53.

rhdevelopers-ci · 2017-08-10T14:09:02Z

...nshift.core/src/org/jboss/tools/openshift/internal/core/server/debug/DebugLaunchConfigs.java

+	 */
+	public boolean isRunning(ILaunchConfiguration launchConfiguration) {
+		return getLaunches()
+				.filter(l -> !l.isTerminated() && launchMatches(l, launchConfiguration))


Replace this "filter().findFirst().isPresent()" chain with "anyMatch()"

adietish · 2017-08-10T14:51:22Z

...nshift.core/src/org/jboss/tools/openshift/internal/core/server/debug/OpenShiftDebugMode.java

+		if (dc == null) {
+			throw new CoreException(StatusFactory.errorStatus(
+					OpenShiftCoreActivator.PLUGIN_ID, "No deployment config present that can be updated."));
+		}


this complaint is false! the "|" is used on purpose so that both bethods are ALWAYS executed.

rhdevelopers-ci · 2017-08-10T15:08:51Z

...nshift.core/src/org/jboss/tools/openshift/internal/core/server/debug/OpenShiftDebugMode.java

+	public static DebugContext createContext(IControllableServerBehavior behaviour, String devmodeKey, String debugPortKey, String debugPort) {
+		Assert.isNotNull(behaviour);
+
+		DebugContext context = new DebugContext(behaviour, 


Immediately return this expression instead of assigning it to the temporary variable "context".

rhdevelopers-ci · 2017-08-10T15:08:53Z

...nshift.core/src/org/jboss/tools/openshift/internal/core/server/debug/DebugLaunchConfigs.java

+	}
+
+	public void setupRemoteDebuggerLaunchConfiguration(ILaunchConfigurationWorkingCopy workingCopy, IProject project, int debugPort) 
+			throws CoreException {


Remove the declaration of thrown exception 'org.eclipse.core.runtime.CoreException', as it cannot be thrown from method's body.

rhdevelopers-ci · 2017-08-10T15:08:53Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

-		IControllableServerBehavior behavior = server.getAdapter(IControllableServerBehavior.class);
-		return behavior;
+	protected void launchServerProcess(OpenShiftServerBehaviour beh, ILaunch launch,
+			IProgressMonitor monitor) {


Remove this unused method parameter "monitor".

rhdevelopers-ci · 2017-08-10T15:08:54Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

+		monitor.subTask("waiting for docker image to become available...");
+		while (!monitor.isCanceled()) {
+			while (!metadata.load() 
+					&& sleep(RECHECK_DELAY)) {


Either remove or fill this block of code.

rhdevelopers-ci · 2017-08-10T15:08:54Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

 				ILaunch debuggerLaunch = attachRemoteDebugger(server, localPort, monitor);
-				if( debuggerLaunch != null ) {
+				if (debuggerLaunch != null) {


Remove this expression which always evaluates to "true"

rhdevelopers-ci · 2017-08-10T15:08:55Z

...re/src/org/jboss/tools/openshift/internal/core/server/resources/ResourceChangePublisher.java

-				   (kind == IncrementalProjectBuilder.FULL_BUILD) || 
-				   ((kind == IncrementalProjectBuilder.AUTO_BUILD && 
-						ResourcesPlugin.getWorkspace().isAutoBuilding()));
+				   kind == IncrementalProjectBuilder.INCREMENTAL_BUILD 


Immediately return this expression instead of assigning it to the temporary variable "eventOccurred".

rhdevelopers-ci · 2017-08-10T15:08:55Z

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

+		return images.iterator().next();
+	}
+
+	private String getImageStreamTag(String imageDigest, String imageRef, String namespace, Connection connection) {


Remove this unused private "getImageStreamTag" method.
Remove this unused method parameter "imageRef".

rhdevelopers-ci · 2017-08-10T15:16:17Z

...nshift.core/src/org/jboss/tools/openshift/internal/core/server/debug/OpenShiftDebugMode.java

+		}
+
+		if (updateDebugmode(dc, context, monitor)
+				| updateDevmode(dc, context, monitor)) {


Correct this "|" to "||".

rhdevelopers-ci · 2017-08-10T15:16:18Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

-
-	private void stopDebugging(IReplicationController rc, DebuggingContext debugContext, IProgressMonitor monitor)
-			throws CoreException {
+	private void stopDebugging(DebugContext debugContext, IProgressMonitor monitor) {


Remove this unused method parameter "monitor".

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

rhdevelopers-ci · 2017-08-10T16:08:25Z

...shift.core/src/org/jboss/tools/openshift/core/server/behavior/OpenShiftLaunchController.java

-			debugContext.setDebugPort(8787);//TODO get default port from server settings?
-		}
+	protected void startDebugging(OpenShiftServerBehaviour behaviour, DebugContext context,
+			IProgressMonitor monitor) {


Remove this unused method parameter "monitor".

rhdevelopers-ci · 2017-08-10T16:08:26Z

SonarQube analysis reported 61 issues

1 blocker
2 critical
36 major
17 minor
5 info

Watch the comments in this conversation to review them.

7 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

OpenShiftLaunchController.java#L490: Remove this expression which always evaluates to "true"
ProgressJob.java#L18: Take the required action to fix the issue indicated by this comment.
ProgressJob.java#L55: Either re-interrupt this method or rethrow the "InterruptedException".
PushImageToRegistryJob.java#L69: Take the required action to fix the issue indicated by this comment.
ResourceChangePublisher.java#L181: Remove this unused "delta" private field.
ResourceChangePublisher.java#L184: Remove this unused "changes" private field.
DockerConfigMetaData.java#L35: Complete the task associated to this TODO comment.

adietish force-pushed the JBIDE-24184 branch 12 times, most recently from 59d1780 to a79af5c Compare June 26, 2017 21:44

adietish force-pushed the JBIDE-24184 branch 4 times, most recently from 714d04f to 606e098 Compare July 3, 2017 20:23

adietish force-pushed the JBIDE-24184 branch 2 times, most recently from 8d412e4 to f0ba3c6 Compare July 4, 2017 21:03

bdshadow previously requested changes Jul 7, 2017

View reviewed changes

bdshadow reviewed Jul 10, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch from 1a8370f to e2ee493 Compare July 10, 2017 21:06

adietish force-pushed the JBIDE-24184 branch from e2ee493 to 17f3028 Compare July 10, 2017 23:57

rhdevelopers-ci reviewed Jul 11, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch from 5411924 to cd23aa2 Compare August 9, 2017 23:53

rhdevelopers-ci reviewed Aug 10, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch 3 times, most recently from 605e45e to 896303f Compare August 10, 2017 14:03

rhdevelopers-ci reviewed Aug 10, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch 4 times, most recently from 7808af3 to 9b82489 Compare August 10, 2017 14:46

adietish commented Aug 10, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch 2 times, most recently from 62ee9a2 to b5cc2c1 Compare August 10, 2017 14:56

rhdevelopers-ci reviewed Aug 10, 2017

View reviewed changes

adietish force-pushed the JBIDE-24184 branch 2 times, most recently from 33515d0 to ae953c6 Compare August 10, 2017 15:44

[JBIDE-24184] get debug port and devmode switch from image

ae953c6

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

rhdevelopers-ci reviewed Aug 10, 2017

View reviewed changes

adietish merged commit d391585 into jbosstools:master Aug 10, 2017

adietish deleted the JBIDE-24184 branch August 10, 2017 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JBIDE-24184] get debug port and devmode switch from image #1518

[JBIDE-24184] get debug port and devmode switch from image #1518

adietish commented Jun 19, 2017 •

edited

adietish commented Jul 4, 2017

bdshadow left a comment

bdshadow Jul 7, 2017

bdshadow Jul 7, 2017

adietish Jul 7, 2017

bdshadow Jul 7, 2017

bdshadow Jul 10, 2017

bdshadow Jul 7, 2017

adietish Jul 7, 2017

bdshadow Jul 10, 2017

adietish Jul 10, 2017

bdshadow commented Jul 10, 2017

adietish commented Jul 10, 2017 •

edited

rhdevelopers-ci Jul 11, 2017

rhdevelopers-ci Jul 11, 2017

rhdevelopers-ci Jul 11, 2017

rhdevelopers-ci Jul 11, 2017

rhdevelopers-ci Jul 11, 2017

adietish commented Aug 9, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017 •

edited

adietish Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci Aug 10, 2017

rhdevelopers-ci commented Aug 10, 2017

[JBIDE-24184] get debug port and devmode switch from image #1518

[JBIDE-24184] get debug port and devmode switch from image #1518

Conversation

adietish commented Jun 19, 2017 • edited

Pull Request Checklist

General

Code

Testing

Function

adietish commented Jul 4, 2017

bdshadow left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdshadow commented Jul 10, 2017

adietish commented Jul 10, 2017 • edited

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

Choose a reason for hiding this comment

adietish commented Aug 9, 2017

Choose a reason for hiding this comment

rhdevelopers-ci Aug 10, 2017 • edited

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

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

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

rhdevelopers-ci commented Aug 10, 2017

7 extra issues

adietish commented Jun 19, 2017 •

edited

adietish commented Jul 10, 2017 •

edited

rhdevelopers-ci Aug 10, 2017 •

edited