Release configuration and archiving VSTS logs improvement #189

zackliu · 2017-12-05T02:54:15Z

Support using credentials Id and dropdown list to choose Team project and Release definition.
Auto generate credentials (if cannot using the existing one) for backwards compatibility.
Add a release logs link in build page.

kasubram · 2017-12-05T04:42:11Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+                listener.getLogger().printf("Waiting for release to be finished...%n");
+                while (true) {
+                    String status = releaseManagementHttpClient.GetReleaseStatus(this.projectName, object.getString("id"));
+                    if (status.equals("finished")) {


what if someone cancelled the release or deleted it. This will forever check the status. We should handle other status as well.

Also, what if there is a post approval which has not been approved for days. This will continue to check for that many days. We should also have a timeout to stop checking.

For one of the above reason if the release is not succeeded and we stopped checking for status and no logs uploaded to jenkins workspace, in that case whenever the jenkins build page is opened we can check once again to see the current status of that release and update the jenkins workspace. is this achievable?

I will check more status and add a time watcher. Do you know where to get documents about the all types of status. I can't find it.
For the last suggestion, I think it's hard to achieve. Because the environment may be changed a lot after build. The workspace may no longer exists after build (User may use dynamic slave and once the build is over, the slave will be deleted) or the configuration has changed. So deal with the old builds may cause unexpected issues. Thus, I think it's make sense to skip the unachievable logs.

It is there in Microsoft.VisualStudio.Services.ReleaseManagement.WebApi.EnvironmentStatus
Possible status are,
Undefined, NotStarted, InProgress, Succeeded, Canceled, Rejected, Queued, Scheduled, PartiallySucceeded

jpricket · 2018-01-23T14:50:48Z

@zackliu - The changes to the TfsConfiguration class seem fine, but the last commit is failing to build. The errors appear to be related to CheckStyle. Please correct the build errors so that we can merge this successfully.

zackliu · 2018-01-23T15:01:02Z

@jpricketMSFT Could you please provide all the types of release status, I can't find it in docs. Thus I can fix the bug kasubram commented before.

jpricket · 2018-01-23T15:28:19Z

@zackliu I am not familiar with the Release code. @kasubram - Could you please provide all the types of release status?

kasubram · 2018-01-23T16:58:14Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+        this.isArchiveLog = isArchiveLog;
+    }
+
+    private Object readResolve() {


shouldn't this be protected?

kasubram · 2018-01-23T16:58:58Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+
+    private Object readResolve() {
+        if (StringUtils.isNotBlank(collectionUrl)
+                && StringUtils.isNotBlank(username)


also if its VSTS and we are using PAT, the username will be empty.

kasubram · 2018-01-23T17:18:07Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+            if (StringUtils.isNotBlank(this.destinationPath)) {
+                listener.getLogger().printf("Waiting for release to be finished...%n");
+                while (true) {
+                    String status = releaseManagementHttpClient.GetReleaseStatus(this.projectName, object.getString("id"));


change it to IsReleaseSucceded? and return a bool?

kasubram · 2018-01-23T17:24:58Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementHttpClient.java

+        {
+            throw new ReleaseManagementException(ex);
+        }
+        catch(IOException ex)


we are already dealing with the generic Exception. Do we need to catch the HttpException and IOException separately?

I don't think it's useful to catch different types of exceptions but throw the same type in the end. However, I just follow the previous functions like ExecuteGetMethod or ExecutePostmethod. So, do we need to also change these functions to just catch generic Exception.

I am not sure how we ended up having that code to catch different exception but finally throw the same exception. Please fix your method just to handle generic exception. Get and Post method I will fix it later.

kasubram · 2018-01-23T17:48:10Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementHttpClient.java

+    {
+        String url = this.accountUrl + "/_apis/projects?api-version=1.0";
+        String response = this.ExecuteGetMethod(url);
+        TeamProject teamProject = new Gson().fromJson(response, TeamProject.class);


Can we do something like,
values = new JSONObject(response).getString("value");
List projects = new Gson().fromJson(values, TeamProject[].class);

The teamproject object is not really representing the teamproject. The api returns a list of teamproject and a count. Looks like we want an intermediate object to represent that, but the final outcome we are looking is List. Looks like just parsing the value field is good enough.

kasubram

Sorry for the delay, have left few more comments, please take a look and let me know. Did you test back compat scenario. (install the old plugin and create a post build action and update to new plugin version and see if everything works fine)

kasubram · 2018-01-29T02:15:28Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

@@ -163,6 +241,36 @@ void CreateRelease(
            JSONObject object = new JSONObject(response);
            listener.getLogger().printf("Release Name: %s%n", object.getString("name"));
            listener.getLogger().printf("Release id: %s%n", object.getString("id"));
+
+            if (StringUtils.isNotBlank(this.destinationPath)) {


also can we wait for release completion only if isArchiveLog is set to true. with the current change it always waits for release completion whether we archive or not. If someone does not want to wait for release completion, he could uncheck isArchiveLog.

In the current change, if user leave destination path blank, we won't wait for release completion. Do you means that if user uncheck isArchiveLog, whether he fills in destination path or not, we won't download logs for he.

zackliu · 2018-01-30T02:52:21Z

@kasubram CI build failed because it said File does not end with a newline in RemoteChangesetVersionCommand.java and TfsUserLookup.java. But actually I didn't change these two files and I found there is one new line at the end of these files.
The Checkstyle passed in my local build, and I noticed a lot of build failed in CI because of this issue. So, any solutions?

kasubram · 2018-01-31T01:59:50Z

tfs/src/main/java/hudson/plugins/tfs/TeamCollectionConfiguration.java

+
+        if (!SystemCredentialsProvider.getInstance().getDomainCredentialsMap().containsKey(requirement)) {
+            generateDomain(hostName);
+        }


leave a space after the brace

kasubram · 2018-01-31T02:02:08Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+                for (ReleaseDefinition releaseDefinition : releaseDefinitions) {
+                    listBoxModel.add(releaseDefinition.getName());
+                }
+            } catch (ReleaseManagementException ignore) {


kasubram · 2018-01-31T02:03:00Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementHttpClient.java

+
+    public void GetReleaseLogs(String project, String releaseId, BuildListener listener) throws ReleaseManagementException
+    {
+        String url = this.accountUrl + project + "/_apis/release/releases/" + "2" + "/logs?api-version=4.0-preview.2";


aren't you supposed to use the releaseId instead of hardcoding "2"?

kasubram · 2018-01-31T02:50:58Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementHttpClient.java

+        InputStream inputStream = this.ExecuteGetMethodAsStream(url);
+
+        try {
+            FileOutputStream out = new FileOutputStream("/home/chenyl/tmp.zip");


please remove this home path. You need use File.CreateTempFile.

You're seeing codes of the first commit. It's already different in the latest commit.

kasubram · 2018-01-31T02:51:13Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementHttpClient.java

+            IOUtils.copy(inputStream, out);
+            out.close();
+        } catch (Exception e) {
+


Please log exception

zackliu · 2018-02-01T02:18:28Z

@kasubram As we discussed, I removed archiving logs and add a link to VSTS logs page.
I updated the summary and screenshots, thus you can have a brief understanding.

kasubram · 2018-02-02T06:38:31Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+
+    private static final long serialVersionUID = -760016860995557L;
+
+    private static final long RELEASE_TIMEOUT_MINUTES = 120;


please remove the unused variables

kasubram · 2018-02-02T06:38:58Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

@@ -163,6 +233,9 @@ void CreateRelease(
            JSONObject object = new JSONObject(response);
            listener.getLogger().printf("Release Name: %s%n", object.getString("name"));
            listener.getLogger().printf("Release id: %s%n", object.getString("id"));
+            String logLink = this.collectionUrl + this.projectName


can we call it releaseUrl?

kasubram · 2018-02-02T06:40:57Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

@@ -163,6 +233,9 @@ void CreateRelease(
            JSONObject object = new JSONObject(response);
            listener.getLogger().printf("Release Name: %s%n", object.getString("name"));
            listener.getLogger().printf("Release id: %s%n", object.getString("id"));
+            String logLink = this.collectionUrl + this.projectName
+                    + "/_release?releaseId=" + object.getString("id") + "&_a=release-logs";


Please do not construct the url by hand. In the release object under _link property you will notice "self" and 'web" keys. Use the url under the "web" key.

kasubram · 2018-02-02T06:41:39Z

tfs/src/main/resources/hudson/plugins/tfs/rm/LogSummaryAction/summary.jelly

@@ -0,0 +1,6 @@
+<?jelly escape-by-default='true'?>


Can we call it release summary?

kasubram · 2018-02-02T06:42:10Z

tfs/src/main/java/hudson/plugins/tfs/rm/LogSummaryAction.java

+public class LogSummaryAction extends InvisibleAction {
+    private String projectName;
+    private int buildNo;
+    private String logLink;


releaseLink?

kasubram · 2018-02-02T06:42:23Z

tfs/src/main/java/hudson/plugins/tfs/rm/LogSummaryAction.java

+ */
+
+@ExportedBean
+public class LogSummaryAction extends InvisibleAction {


can we call it ReleaseSummaryAction?

kasubram · 2018-02-02T09:24:06Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

@@ -163,6 +232,8 @@ void CreateRelease(
            JSONObject object = new JSONObject(response);


are we not converting to ReleaseDetails? if so then ReleaseDetails is not used anymore. If its not used anymore please remove the ReleaseDetails type.

kasubram · 2018-02-02T09:37:00Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseSummaryAction.java

+import org.kohsuke.stapler.export.ExportedBean;
+
+/**
+ * Show release logs link in build page.


show release link..

kasubram · 2018-02-02T09:38:03Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+                for (Project project : projects) {
+                    listBoxModel.add(project.getName());
+                }
+            } catch (ReleaseManagementException ignored) {


please log.. otherwise we will have tough time to debug..

kasubram · 2018-02-02T09:38:22Z

tfs/src/main/java/hudson/plugins/tfs/rm/ReleaseManagementCI.java

+                this.credentialsId
+                        = TeamCollectionConfiguration.setCredentials(hostName, username, password.getPlainText());
+            } catch (Exception ignore) {
+


kasubram

One concern I have is, we do not log the exception. It will be hard to debug. Please go ahead and add logs. Otherwise changes are looking good..

zackliu · 2018-02-07T01:31:03Z

@kasubram Seems the PR has passed the CI. Could we merge it?

GitHubSriramB · 2018-02-07T06:56:36Z

tfs/src/main/java/hudson/plugins/tfs/TeamCollectionConfiguration.java

+        try {
+            SystemCredentialsProvider.getInstance().save();
+        } catch (IOException ignore) {
+


add a warning log?

zackliu added 4 commits November 17, 2017 17:06

Change configuration ways in Trigger release in TFS/Team Service

2df5d42

Finish archive logs from VSTS

e9abe2e

Update help file for ReleaseManagementCI

eabfbfa

Remove wildcard

6ef9c3e

kasubram reviewed Dec 5, 2017

View reviewed changes

kasubram reviewed Jan 23, 2018

View reviewed changes

kasubram requested changes Jan 23, 2018

View reviewed changes

kasubram reviewed Jan 29, 2018

View reviewed changes

Add timeout check and allow empty username

a429262

kasubram reviewed Jan 31, 2018

View reviewed changes

Remove archive logs and add a log link

af3aec0

kasubram reviewed Feb 2, 2018

View reviewed changes

Rename log to release

f742cfa

kasubram reviewed Feb 2, 2018

View reviewed changes

Delete releaseDetails

c6422c6

kasubram reviewed Feb 2, 2018

View reviewed changes

kasubram approved these changes Feb 2, 2018

View reviewed changes

zackliu added 3 commits February 5, 2018 10:38

Add logs

58da4f2

Merge remote-tracking branch 'jenkinsci/master' into ori_dev

fa223c1

Change back new line

301b142

GitHubSriramB approved these changes Feb 7, 2018

View reviewed changes

Add logs

7a0e846

jpricket merged commit cdda7a0 into jenkinsci:master Feb 7, 2018


		private static final long serialVersionUID = -760016860995557L;

		private static final long RELEASE_TIMEOUT_MINUTES = 120;

		@@ -163,6 +232,8 @@ void CreateRelease(
		JSONObject object = new JSONObject(response);

Release configuration and archiving VSTS logs improvement #189

Release configuration and archiving VSTS logs improvement #189

Conversation

zackliu commented Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpricket commented Jan 23, 2018

zackliu commented Jan 23, 2018

jpricket commented Jan 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasubram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackliu commented Jan 30, 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

zackliu Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackliu commented Feb 1, 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

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

kasubram left a comment

Choose a reason for hiding this comment

zackliu commented Feb 7, 2018

Choose a reason for hiding this comment

zackliu commented Dec 5, 2017 •

edited

Loading

zackliu Jan 31, 2018 •

edited

Loading