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

Add an "opt-out" global switch to retain second fetch #927

Merged
merged 11 commits into from
Jul 4, 2020

Conversation

rishabhBudhouliya
Copy link
Contributor

@rishabhBudhouliya rishabhBudhouliya commented Jul 2, 2020

PR-904 Fix redundant fetch

With the latest fix on redundant fetch issue [JENKINS-49757], the
second fetch in the checkout step is avoided for cases where the
second fetch is not adding any new repository information to the
existing git repository.

Enabling this switch will allow the user to preserve the second
fetch and remedy any compatibility issues.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is simply a reminder of what we are going to look for before merging your code. If a checkbox or line does not apply to this pull request, delete it. We prefer all checkboxes to be checked before a pull request is merged

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

  • New feature (non-breaking change which adds functionality)

With the latest fix on redundant fetch issue [JENKINS-49757], the
second fetch in the checkout step is avoided for cases where the
second fetch is not adding any new repository information to the
existing git repository.

Enabling this switch will allow the user to preserve the second
fetch and remedy any compatibility issues.
@@ -1483,6 +1482,7 @@ public ChangeLogParser createChangeLogParser() {
private boolean useExistingAccountWithSameEmail;
// private GitClientType defaultClientType = GitClientType.GITCLI;
private boolean showEntireCommitSummaryInChanges;
private boolean enablePerformanceImprovement = true; // By default, performance is enabled in git plugin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch was originally created to add performance improvement global switch. This switch has been used to add "opt out" feature for retaining second fetch during the checkout step.

@@ -16,6 +16,9 @@
<f:entry field="showEntireCommitSummaryInChanges">
<f:checkbox title="${%Show the entire commit summary in changes}" name="showEntireCommitSummaryInChanges" checked="${descriptor.showEntireCommitSummaryInChanges}"/>
</f:entry>
<f:entry field="enablePerformanceImprovement">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been changed to allowSecondFetch in the subsequent commits.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

The asymmetry between getter and setter name is likely to disrupt configuration as code and JobDSL.

Could you make them follow the usual naming convention?

private boolean allowSecondFetch;
public boolean isAllowSecondFetch() {}
public void setAllowSecondFetch(boolean allowSecondFetch) {}

Interactive testing seems to show that enabling 'Allow second fetch' is not being preserved for later use in the "Manage Jenkins" page.

The following steps give unexpected results:

  1. Open the "Manage Jenkins" page and find the "Git plugin" section of the page
  2. Enable "Allow second fetch" by clicking the checkbox
  3. Save the configuration
  4. Run a job and confirm the second fetch is visible
  5. Return to the "Manage Jenkins" page and find the "Git plugin" section of the page
  6. Unexpectedly, the "Allow second fetch" checkbox is no longer checked

If you'd like to experiment with modifying the values without using the UI controls, you can use the "Script Console" that is available from "Manage Jenkins" with the following code:

def inst = Jenkins.getInstance()
def desc = inst.getDescriptor("hudson.plugins.git.GitSCM")
println(desc.getGlobalConfigName())
println(desc.getGlobalConfigEmail())
println(desc.isRedundantFetchAllowed())

desc.setGlobalConfigName("Mark Waite")
desc.setAllowSecondFetch(false)

println(desc.getGlobalConfigName())
println(desc.getGlobalConfigEmail())
println(desc.isRedundantFetchAllowed())

README.adoc Show resolved Hide resolved
src/main/java/hudson/plugins/git/GitSCM.java Outdated Show resolved Hide resolved
src/main/java/hudson/plugins/git/GitSCM.java Show resolved Hide resolved
@rishabhBudhouliya
Copy link
Contributor Author

@MarkEWaite I was also interactively testing this change and I am also facing this issue. The checkbox option is not being persisted across multiple build jobs.

I'll debug the instance for RCA.

@rishabhBudhouliya
Copy link
Contributor Author

@MarkEWaite I have done two things:

  • Interactively tested the checkbox, now the selection is persisting in my local instance.
  • Removed redundant and replaced it with second.

@MarkEWaite MarkEWaite merged commit 447fe52 into jenkinsci:master Jul 4, 2020
@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants