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

JBIDE-22303 Server Adapter: Manually set pod deployment path is ignored #1141

Closed
wants to merge 1 commit into from

Conversation

scabanovich
Copy link
Contributor

No description provided.

@fbricon
Copy link
Member

fbricon commented May 5, 2016

Can you update the unit tests for that class, to ensure we don't break that feature in the future?

@adietish
Copy link
Member

adietish commented May 5, 2016

@scabanovich #1133 (which was merged) added a test class for the server adapter model that you can add your test to

@scabanovich
Copy link
Contributor Author

Test is added.

@@ -94,6 +95,17 @@ public void shouldReturnProjectThatHasGitRemoteMatchingBuildConfig() {
assertThat(project).isEqualTo(project2);
}

@Test
public void testPodPathProperty() {
Copy link
Member

@fbricon fbricon May 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did these tests really fail before your fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because useInferredPodPath and podPath parameters were not assigned to class fields in update().

@fbricon
Copy link
Member

fbricon commented May 6, 2016

This doesn't work: http://screencast.com/t/Typn2W7b2nU

@@ -378,6 +380,7 @@ private void updateServer(IServerWorkingCopy server) throws OpenShiftException {
String serverName = OpenShiftServerUtils.getServerName(getService(), getConnection());
String host = getHost(getRoute());
String routeURL = getRouteURL(isSelectDefaultRoute(), getRoute());
String podPath = useInferredPodPath ? this.podPath : "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be

String podPath = useInferredPodPath ? "" : this.podPath;

@fbricon
Copy link
Member

fbricon commented May 6, 2016

Merged as c47cbb5 after fixing the wrong podpath sent to the server.

@adietish, please make sure your ongoing tests check updateServer() properly mutates the server working copy.

@fbricon fbricon closed this May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants