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

Fixes for several issues in JenkinsQueueJob task #8887

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

keljos
Copy link
Member

@keljos keljos commented Nov 20, 2018

Fixes for #7527, a bug in how tree query parameters are used, and for an issue where builds are failing when the network connection is lost while streaming the Jenkins console.

Tested with local deployment and ran L0 tests

…iling when connection is lost while streaming jenkins console.
@@ -276,7 +276,9 @@ export class Job {
if (thisJob.queue.TaskOptions.capturePipeline) {
const downstreamProjects = thisJob.Search.ParsedTaskBody.downstreamProjects || [];
downstreamProjects.forEach((project) => {
new Job(thisJob.queue, thisJob, project.url, null, -1, project.name); // will add a new child to the tree
if (project.color !== 'disabled') {
Copy link
Member Author

Choose a reason for hiding this comment

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

The 'color' field is how Jenkins identifies the status of a project. This field will be set to disabled if the project has been disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, we only add jobs to the tree if they are not disabled. What about the root job -- can it be disabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Attempting to queue a disabled root job results in a 404 from Jenkins, which causes our build to fail right away.

@@ -445,7 +447,7 @@ export class Job {
thisJob.stopWork(0, JobState.Finishing);
}
}
}).auth(thisJob.queue.TaskOptions.username, thisJob.queue.TaskOptions.password, true)
}).auth(thisJob.queue.TaskOptions.username, thisJob.queue.TaskOptions.password, false)
Copy link
Member Author

Choose a reason for hiding this comment

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

This parameter is the 'sendImmeadiately' flag. When set to false it forces the request to retry with the proper authentication headers. See this doc for reference: https://www.npmjs.com/package/request#http-authentication)

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment in code, plz!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@@ -242,7 +242,7 @@ export class JobSearch {
thisSearch.stopWork(0); // found everything we were looking for
return;
} else {
const url: string = util.addUrlSegment(thisSearch.taskUrl, thisSearch.nextSearchBuildNumber + '/api/json?tree=actions,timestamp');
const url: string = util.addUrlSegment(thisSearch.taskUrl, thisSearch.nextSearchBuildNumber + '/api/json?tree=actions[causes[shortDescription,upstreamBuild,upstreamProject,upstreamUrl]],timestamp');
Copy link
Member Author

Choose a reason for hiding this comment

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

We should be specifying the fields we want returned in the actions, else we get the default empty actions block.

@@ -33,7 +33,7 @@ export class JobSearch {
private foundCauses : any[] = []; // all found causes indexed by executableNumber

public Initialized: boolean = false;
public ParsedTaskBody: {downstreamProjects?: {name: string, url: string}[], lastBuild?: {number: number}}; // the parsed task body of the job definition
public ParsedTaskBody: {downstreamProjects?: {name: string, url: string, color: string}[], lastBuild?: {number: number}}; // the parsed task body of the job definition
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just define a named type for this -- would make it easier to read

Copy link
Contributor

@brcrista brcrista Nov 21, 2018

Choose a reason for hiding this comment

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

Suggested change
public ParsedTaskBody: {downstreamProjects?: {name: string, url: string, color: string}[], lastBuild?: {number: number}}; // the parsed task body of the job definition
interface Project {
name: string,
url: string,
color: string
}
interface ParsedTaskBody {
downstreamProjects?: Project[],
lastBuild?: {
number: number
}
}
// Then in the class here:
public ParsedTaskBody: ParsedTaskBody; // the parsed task body of the job definition

@go2guy
Copy link

go2guy commented Nov 27, 2018

Any idea on when this will be released and will it be version 3?

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.

4 participants