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.

Sign up for GitHub

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

List all web apps when Run on Web App #1948

Merged
merged 9 commits into from
Sep 7, 2018
+247 −164
Merged

List all web apps when Run on Web App #1948

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f4f5f42
List all web apps when Run on Web App
Sep 5, 2018
3846422
Resolve comments
Sep 5, 2018
95c3927
Resolve comments
Sep 5, 2018
7e14ec4
Filter windows or linux java web apps to show
Sep 6, 2018
63de65e
Add final modifier for list web apps functions
Sep 6, 2018
5024aa6
Move web app util functions to right place
Sep 7, 2018
ddab6ac
Update util functions according to comments
Sep 7, 2018
2b93e8e
Use switch-case to replace if-else
Sep 7, 2018
0797533
Fix CI
Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions ...uretools-core/src/com/microsoft/azuretools/core/mvp/model/webapp/AzureWebAppMvpModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,8 @@ public List<ResourceEx<WebApp>> listWebAppsOnLinux(@NotNull final String subscri
* List web apps on windows by subscription id.
*/
public List<ResourceEx<WebApp>> listWebAppsOnWindows(@NotNull final String subscriptionId, final boolean force) {
return listWebApps(subscriptionId, force).stream()
return listWebApps(subscriptionId, force)
.stream()
.filter(resourceEx -> OperatingSystem.WINDOWS.equals((resourceEx.getResource().operatingSystem())))
.collect(Collectors.toList());
}
Expand All @@ -402,7 +403,8 @@ public List<ResourceEx<WebApp>> listWebApps(final String subscriptionId, final b
List<ResourceEx<WebApp>> webApps = new ArrayList<>();
try {
final Azure azure = AuthMethodManager.getInstance().getAzureClient(subscriptionId);
webApps = azure.webApps().list().stream()
webApps = azure.webApps().list()
.stream()
.map(app -> new ResourceEx<WebApp>(app, subscriptionId))
.collect(Collectors.toList());
subscriptionIdToWebApps.put(subscriptionId, webApps);
Expand Down Expand Up @@ -471,14 +473,19 @@ public boolean getPublishingProfileXmlWithSecrets(String sid, String webAppId, S
}
}

@Deprecated
public void cleanWebAppsOnWindows() {
Eskibear marked this conversation as resolved.
Show resolved Hide resolved
// todo
// todo: remove the function
// todo: create a new function clearWebAppsCache clear cache web apps
// subscriptionIdToWebAppsOnWindowsMap.clear();
subscriptionIdToWebApps.clear();
}

@Deprecated
public void cleanWebAppsOnLinux() {
// todo
// todo: remove the function
// subscriptionIdToWebAppsOnLinuxMap.clear();
subscriptionIdToWebApps.clear();
}

/**
Expand Down
66 changes: 34 additions & 32 deletions Utils/azuretools-core/src/com/microsoft/azuretools/utils/WebAppUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.microsoft.azure.management.Azure;
import com.microsoft.azure.management.appservice.AppServicePlan;
import com.microsoft.azure.management.appservice.JavaVersion;
import com.microsoft.azure.management.appservice.OperatingSystem;
import com.microsoft.azure.management.appservice.PublishingProfile;
import com.microsoft.azure.management.appservice.WebApp;
import com.microsoft.azure.management.appservice.WebContainer;
Expand All @@ -34,7 +35,6 @@
import com.microsoft.azuretools.authmanage.AuthMethodManager;
import com.microsoft.azuretools.authmanage.models.SubscriptionDetail;
import com.microsoft.azuretools.azurecommons.helpers.NotNull;
import com.microsoft.azuretools.core.mvp.model.ResourceEx;
import com.microsoft.azuretools.sdkmanage.AzureManager;

import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -68,6 +68,7 @@ public class WebAppUtils {
private static final String ROOT = "ROOT";
private static final int FTP_MAX_TRY = 3;
private static final int SLEEP_TIME = 5000; // milliseconds
private static final String DEFAULT_VALUE_WHEN_VERSION_INVALID = "";

@NotNull
public static FTPClient getFtpConnection(PublishingProfile pp) throws IOException {
Expand Down Expand Up @@ -370,55 +371,56 @@ public ResourceGroup getRg() {
}

/**
* app.linuxFxVersion() is the only API we could get the version info of a linux web app
* It returns values like this "Tomcat|8.5-jre8" if it is a linux with web container Tomcat
* @param webApp
* @return jdk version
* app.linuxFxVersion() is the only API we could get the version info of a Linux web app.
* It returns values like "Tomcat|8.5-jre8" if it is a Linux with the web container Tomcat.
*/
public static String getJDKVersion(@NotNull final WebApp webApp) {
try {
if (StringUtils.isEmpty(webApp.linuxFxVersion())) {
return webApp.javaVersion().toString();
final OperatingSystem operatingSystem = webApp.operatingSystem();
if (operatingSystem == OperatingSystem.WINDOWS) {
Copy link
Member

Choose a reason for hiding this comment

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

if-else vs switch-case

Copy link
Member

Choose a reason for hiding this comment

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

I prefer switch-case especially for Enum. But I'm not stick to it.
For me either of the following is ok:
a) (prefer)

switch(os){
case linux:
case windows:
default:
}

b)

if (os == windows) {
// ...
} else if (os == linux) {
// ... 
}

Do you have any idea? @jdneo

Copy link
Member

@jdneo jdneo Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, switch-case is better

Copy link
Contributor Author

@SummerSun SummerSun Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Agree that switch-case is better than if-else.

```
final OperatingSystem operatingSystem = webApp.operatingSystem();
if (operatingSystem == null) {
// just return
}

final String os = operatingSystem.toString.toLowerCase();
switch(os) {
// our logic here
}
```

return webApp.javaVersion().toString();
}
if (operatingSystem == OperatingSystem.LINUX) {
final String linuxVersion = webApp.linuxFxVersion();
if (linuxVersion == null) {
return DEFAULT_VALUE_WHEN_VERSION_INVALID;
}

final String[] linuxVersion = webApp.linuxFxVersion().split("-");
return linuxVersion[1];
} catch (Exception e) {
e.printStackTrace();
final String[] versions = linuxVersion.split("-");
return versions.length == 2 ? versions[1] : DEFAULT_VALUE_WHEN_VERSION_INVALID;
Copy link
Member

Choose a reason for hiding this comment

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

In case versions.length != 2 (means you fail to split the linuxVersion) do you prefer to return a VERSION_INVALID or simply the linuxVersion itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If version.length != 2, I treat it as an invalid version and I prefer to return DEFAULT_VALUE_WHEN_VERSION_INVALID. Any concerns?

}
return "";
return DEFAULT_VALUE_WHEN_VERSION_INVALID;
}

/**
* app.linuxFxVersion() is the only API we could get the version info of a linux web app
* It returns "Tomcat|8.5-jre8" if it is a linux web app with web container Tomcat
* The only web container supported is Tomcat
* If the web app is a Java SE web app, linuxFxVersion() returns "Java|8-jre8"
* @param webApp
* @return web container
* app.linuxFxVersion() is the only API we could get the version info of a Linux web app.
* It returns "Tomcat|8.5-jre8" if it is a Linux web app with the web container Tomcat.
* Tomcat is the only supported web container.
* If the web app is a Java SE web app, which has no web container, linuxFxVersion() returns "Java|8-jre8".
* And we will return N/A for those kind of web apps.
*/
public static String getWebContainer(@NotNull final WebApp webApp) {
try {
if (StringUtils.isEmpty(webApp.linuxFxVersion())) {
return webApp.javaContainer() + " " + webApp.javaContainerVersion();
final OperatingSystem operatingSystem = webApp.operatingSystem();
if (operatingSystem == OperatingSystem.WINDOWS) {
Copy link
Member

Choose a reason for hiding this comment

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

Following same if-else vs switch-case discussion above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied in above thread.

return String.join(" ", webApp.javaContainer(), webApp.javaContainerVersion());
}
if (operatingSystem == OperatingSystem.LINUX) {
final String linuxVersion = webApp.linuxFxVersion();
final String[] versions = linuxVersion.split("-");
if (versions.length != 2) {
return DEFAULT_VALUE_WHEN_VERSION_INVALID;
}

final String[] linuxVersion = webApp.linuxFxVersion().split("-");
if (linuxVersion[0].toLowerCase().contains("tomcat")) {
if (StringUtils.containsIgnoreCase(versions[0], "tomcat")) {
Copy link
Member

Choose a reason for hiding this comment

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

Why compare it with "tomcat"? What other values do you want to filter out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as the comment explains, Tomcat is the only supported web container when web app runs on Linux.

Copy link
Member

Choose a reason for hiding this comment

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

Then what's the difference with returning version[0] directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the Windows runtime has so different structure with Linux runtime. Will update the table from in next PR.

Name JDK Web Container Resource Group

to

Name OS Runtime Resource Group

Track this in isssue #1968

return versions[0].replace("|", " ");
} else {
return "N/A";
}
return linuxVersion[0].replace("\\|", " ");
} catch (Exception e) {
e.printStackTrace();
}
return "";
return DEFAULT_VALUE_WHEN_VERSION_INVALID;
}

/**
* Check if the web app is windows or linux java configured web app.
* Check if the web app is a Windows or Linux Java configured web app.
* Docker web apps are not included.
* @param webApp
* @return
*/
public static boolean isJavaWebApp(@NotNull WebApp webApp) {
return webApp.javaVersion() != JavaVersion.OFF ||
Expand Down