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

Fix tfs push bug #1

Closed
wants to merge 11 commits into from
Closed

Fix tfs push bug #1

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c4a2399
Fix tfs push bug
smile21prc Jun 22, 2017
83fb492
Cleanup
smile21prc Jun 22, 2017
c345498
Still need this iteration
smile21prc Jun 22, 2017
38d1c0f
Clean up spaces
smile21prc Jun 22, 2017
83fdbb0
Revert "Clean up spaces"
smile21prc Jun 22, 2017
45b2fd4
Revert "Still need this iteration"
smile21prc Jun 22, 2017
959dc21
Remove unnecessary continue
smile21prc Jun 22, 2017
7d6d684
Address review comments.
smile21prc Jun 23, 2017
cfd55e1
Add import of StringUtils.
smile21prc Jun 23, 2017
5021220
Addressed Matt's comments.
smile21prc Jun 26, 2017
447d380
Addressed more of Matt's comments.
smile21prc Jun 26, 2017
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
4 changes: 2 additions & 2 deletions tfs/src/main/java/hudson/plugins/tfs/TeamPushTrigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ public void run() {
boolean shouldSchedule = bypassPolling;
String changesDetected = "";
if (!bypassPolling) {
if (runPolling()) {
if (runPolling() || gitCodePushedEventArgs.commit != null) {
Copy link

Choose a reason for hiding this comment

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

I would prefer...
|| StringUtils.isNotBlank(gitCodePushedEventArgs.commit)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good suggestion Jason, I'll fix and verify it.

changesDetected = "SCM changes detected in " + job.getFullDisplayName() + ". ";
shouldSchedule = true;
}
}
else {
changesDetected = "Polling bypassed for " + job.getFullDisplayName() + ". ";;
changesDetected = "Polling bypassed for " + job.getFullDisplayName() + ". ";
}
if (shouldSchedule) {
final SCMTriggerItem p = job();
Expand Down
148 changes: 65 additions & 83 deletions tfs/src/main/java/hudson/plugins/tfs/model/AbstractHookEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,68 @@ static JSONObject fromResponseContributors(final List<GitStatus.ResponseContribu
return result;
}

GitStatus.ResponseContributor triggerJob(final GitCodePushedEventArgs gitCodePushedEventArgs, final List<Action> actions, final boolean bypassPolling, final Item project, final SCMTriggerItem scmTriggerItem) {
GitStatus.ResponseContributor gitResponse = null;

if (!(project instanceof AbstractProject && ((AbstractProject) project).isDisabled())) {
if (project instanceof Job) {
// TODO: Add default parameters defined in the job
Copy link

Choose a reason for hiding this comment

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

Is it really not queueing it with parameters?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Verified the jobs being kicked off do use customized params, so this comment isn't needed any more, removed it.

final Job job = (Job) project;
final int quietPeriod = scmTriggerItem.getQuietPeriod();

boolean triggered = false;
if (!triggered) {
Copy link

Choose a reason for hiding this comment

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

This is always false here

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, this is the code from original plugin. I've removed this unnecessary if statement.

final TeamPluginGlobalConfig config = TeamPluginGlobalConfig.get();
if (config.isEnableTeamPushTriggerForAllJobs()) {
triggered = true;
final SCMTrigger scmTrigger = TeamEventsEndpoint.findTrigger(job, SCMTrigger.class);
if (scmTrigger != null && scmTrigger.isIgnorePostCommitHooks()) {
// job has explicitly opted out of hooks
triggered = false;
Copy link

Choose a reason for hiding this comment

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

There is a lot of use of triggered in here in sort of confusing ways. I think the if (triggered) block below should be inlined into this block (invert the above if condition), returning the new gitResponse directly rather than waiting for the end of the series of if statements.

Copy link
Owner Author

@smile21prc smile21prc Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Refined the logic, and removed "triggered" var entirely as it becomes unnecessary. Also changed to return directly instead of waiting.

}
}
if (triggered) {
final TeamPushTrigger trigger = new TeamPushTrigger(job);
trigger.execute(gitCodePushedEventArgs, actions, bypassPolling);
if (bypassPolling) {
gitResponse = new TeamEventsEndpoint.ScheduledResponseContributor(project);
}
else {
gitResponse = new TeamEventsEndpoint.PollingScheduledResponseContributor(project);
}
}
}
if (!triggered) {
Copy link

Choose a reason for hiding this comment

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

If you inline the if (triggered) block, then this if (!triggered) becomes unnecessary since it's always false if you get here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, removed "triggered" var entirely as it becomes unnecessary.

final SCMTrigger scmTrigger = TeamEventsEndpoint.findTrigger(job, SCMTrigger.class);
if (scmTrigger != null && !scmTrigger.isIgnorePostCommitHooks()) {
// queue build without first polling
final Cause cause = new TeamHookCause(gitCodePushedEventArgs.commit);
final CauseAction causeAction = new CauseAction(cause);
final Action[] actionArray = ActionHelper.create(actions, causeAction);
scmTriggerItem.scheduleBuild2(quietPeriod, actionArray);
gitResponse = new TeamEventsEndpoint.ScheduledResponseContributor(project);
triggered = true;
Copy link

Choose a reason for hiding this comment

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

Again you could just return gitResponse directly from this block

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good suggestion, did that.

}
}
if (!triggered) {
Copy link

Choose a reason for hiding this comment

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

Which would make this always false again.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agree, removed this unnecessary statement.

final TeamPushTrigger pushTrigger = TeamEventsEndpoint.findTrigger(job, TeamPushTrigger.class);
if (pushTrigger != null) {
pushTrigger.execute(gitCodePushedEventArgs, actions, bypassPolling);
final GitStatus.ResponseContributor response;
if (bypassPolling) {
gitResponse = new TeamEventsEndpoint.ScheduledResponseContributor(project);
}
else {
gitResponse = new TeamEventsEndpoint.PollingScheduledResponseContributor(project);
}
triggered = true;
Copy link

Choose a reason for hiding this comment

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

And again return directly from this

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

}
}
}
}
return gitResponse;
}

// TODO: it would be easiest if pollOrQueueFromEvent built a JSONObject directly
List<GitStatus.ResponseContributor> pollOrQueueFromEvent(final GitCodePushedEventArgs gitCodePushedEventArgs, final List<Action> actions, final boolean bypassPolling) {
List<GitStatus.ResponseContributor> result = new ArrayList<GitStatus.ResponseContributor>();
Expand Down Expand Up @@ -111,90 +173,10 @@ List<GitStatus.ResponseContributor> pollOrQueueFromEvent(final GitCodePushedEven
if (scmTriggerItem == null) {
continue;
}
for (final SCM scm : scmTriggerItem.getSCMs()) {
if (!(scm instanceof GitSCM)) {
continue;
}
final GitSCM git = (GitSCM) scm;
scmFound = true;

for (final RemoteConfig repository : git.getRepositories()) {
boolean repositoryMatches = false;
for (final URIish remoteURL : repository.getURIs()) {
if (UriHelper.areSameGitRepo(uri, remoteURL)) {
repositoryMatches = true;
totalRepositoryMatches++;
break;
}
}

if (!repositoryMatches || git.getExtensions().get(IgnoreNotifyCommit.class)!=null) {
continue;
}

if (!(project instanceof AbstractProject && ((AbstractProject) project).isDisabled())) {
if (project instanceof Job) {
// TODO: Add default parameters defined in the job
final Job job = (Job) project;
final int quietPeriod = scmTriggerItem.getQuietPeriod();

boolean triggered = false;
if (!triggered) {
final TeamPluginGlobalConfig config = TeamPluginGlobalConfig.get();
if (config.isEnableTeamPushTriggerForAllJobs()) {
triggered = true;
final SCMTrigger scmTrigger = TeamEventsEndpoint.findTrigger(job, SCMTrigger.class);
if (scmTrigger != null && scmTrigger.isIgnorePostCommitHooks()) {
// job has explicitly opted out of hooks
triggered = false;
}
}
if (triggered) {
final TeamPushTrigger trigger = new TeamPushTrigger(job);
trigger.execute(gitCodePushedEventArgs, actions, bypassPolling);
final GitStatus.ResponseContributor response;
if (bypassPolling) {
response = new TeamEventsEndpoint.ScheduledResponseContributor(project);
}
else {
response = new TeamEventsEndpoint.PollingScheduledResponseContributor(project);
}
result.add(response);
}
}
if (!triggered) {
final SCMTrigger scmTrigger = TeamEventsEndpoint.findTrigger(job, SCMTrigger.class);
if (scmTrigger != null && !scmTrigger.isIgnorePostCommitHooks()) {
// queue build without first polling
final Cause cause = new TeamHookCause(commit);
final CauseAction causeAction = new CauseAction(cause);
final Action[] actionArray = ActionHelper.create(actions, causeAction);
scmTriggerItem.scheduleBuild2(quietPeriod, actionArray);
result.add(new TeamEventsEndpoint.ScheduledResponseContributor(project));
triggered = true;
}
}
if (!triggered) {
final TeamPushTrigger pushTrigger = TeamEventsEndpoint.findTrigger(job, TeamPushTrigger.class);
if (pushTrigger != null) {
pushTrigger.execute(gitCodePushedEventArgs, actions, bypassPolling);
final GitStatus.ResponseContributor response;
if (bypassPolling) {
response = new TeamEventsEndpoint.ScheduledResponseContributor(project);
}
else {
response = new TeamEventsEndpoint.PollingScheduledResponseContributor(project);
}
result.add(response);
triggered = true;
}
}
if (triggered) {
break;
}
}
}
}
GitStatus.ResponseContributor triggerResult = triggerJob(gitCodePushedEventArgs, actions, bypassPolling, project, scmTriggerItem);
if (triggerResult != null) {
result.add(triggerResult);
}
}
if (!scmFound) {
Expand Down