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

[JENKINS-19022] purge deleted branches from BuildData #312

Merged
merged 2 commits into from Mar 25, 2015

Conversation

Projects
None yet
5 participants
@ndeloof
Copy link
Member

commented Mar 23, 2015

For team using a topic-branch workflow, BuildData is an always growing Map of (legacy) branches, copied from build to build. This enhancement to PruneStaleBranch removes obsolete entries from current BuildData object based on existing branches on remote repositori(es)

@reviewbybees

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Mar 23, 2015

Another part of big BuildData "fixing" is to allow choosing 'prevBuildData' through BuildChooser strategy. IRC log from talk with kohsuke https://gist.github.com/KostyaSha/1de94870ae254520f218 If you have time you can do it instead of me.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2015

@KostyaSha thanks for the link. This PR does not expect to "fix" JENKINS-19022 and BuildData issue, would need a fresh new mechanism to avoid such duplication, but all my refactoring attempts failed so far.

* Remove branches from BuildData that have been seen in the past but do not exist anymore
* @param branches all branches available in current repository state
*/
public void purgeAllBranchesBut(Set<Branch> branches) {

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 24, 2015

Member

Maybe purgeStaleBranches(Set<Branch> keepBranches) ?

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2015

@KostyaSha about your discussion with KK, I've tried few times to remove per-build BuildData and use a centralized storage. Best attempt so far is https://github.com/ndeloof/git-plugin/tree/BuildData-sucks
BUT I had to drop some (exotic?) feature and some tests still fail

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

@ndeloof could you PR to see real changes?

@alecharp

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

👍

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

@ndeloof for optimising PR builds i tried to specify only 1 branch for refspec and the same branch for branch specifier. But according to log git plugin fetches all branches always (didn't have time to investigate why). May this purge work wrong if git plugin will skip fetching all branches?

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2015

I expected such potential side-effect, so call to this purge method is part of PruneStaleBranch optional extension, not mainstream use.

project.poll(listener).hasChanges());
FreeStyleBuild b3 = build(project, Result.SUCCESS, commitFile3);
data = b3.getAction(BuildData.class);
assertTrue("topic-1 branch should have been purged", !data.buildsByBranchName.containsKey("origin/topic-1"));

This comment has been minimized.

Copy link
@jglick

jglick Mar 24, 2015

Member

assertFalse might be more idiomatic. (Or you can go the Hamcrest route.)

Would it make more sense to just assert something about the textual contents of build.xml, so as to avoid forcing test references to BuildData? IIUC the problem is not about some incorrect behavior, it is just about the build metadata steadily growing.

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 24, 2015

Author Member

The goal here is to check deleted branch in repo are well deleted in BuilData structure, to reduce memory footprint. I don't get why checking on build.xml would be more relevant.

This comment has been minimized.

Copy link
@jglick

jglick Mar 24, 2015

Member

Just that it would avoid a hard reference to BuildData.

@ndeloof ndeloof force-pushed the ndeloof:BuildData branch from 253d841 to 63b2b92 Mar 25, 2015

@ndeloof ndeloof merged commit 63b2b92 into jenkinsci:master Mar 25, 2015

1 check was pending

Jenkins Jenkins is validating pull request ...
Details

@ndeloof ndeloof deleted the ndeloof:BuildData branch Mar 25, 2015

@jglick

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

FTR: reverted in 336c178 and 91298be

@@ -1637,6 +1637,43 @@ public void testSha1NotificationBranches() throws Exception {
notifyAndCheckBranch(project, commit1, branchName, 1, git);
}


@Issue("JENKINS-17348")

This comment has been minimized.

Copy link
@jglick

jglick Oct 20, 2016

Member

Was this actually the right number? Sounds unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.