Skip to content

Conversation

hgschmie
Copy link
Contributor

@hgschmie hgschmie commented Jul 12, 2019

Version 3.0.0 started to look at "ahead" and "behind" branches. To do
so, it executes a Fetch command to the repo. This is unfortunate
because it introduces quite a bit of a delay when building, especially
when the machine is offline (e.g. on a plane). Also, it sets off
various alerts because the previous version did not do any remote
access.

This PR introduces an "offline" mode (disabled by default) that allows
turning off all commands that are trying to reach out to
remotes. Currently, that is only the fetch command which is executed
when trying to determine "ahead" and "behind". Any future operation
that is accessing remote repos should also check this flag.

Context

Contributor Checklist

  • Added relevant integration or unit tests to verify the changes
  • Update the Readme or any other documentation (including relevant Javadoc)
  • Ensured that tests pass locally: mvn clean package
  • Ensured that the code meets the current checkstyle coding style definition: mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B

Henning Schmiedehausen added 2 commits July 12, 2019 15:07
Version 3.0.0 started to look at "ahead" and "behind" branches. To do
so, it executes a Fetch command to the repo. This is unfortunate
because it introduces quite a bit of a delay when building, especially
when the machine is offline (e.g. on a plane). Also, it sets off
various alerts because the previous version did not do any remote
access.

This PR introduces an "offline" mode (disables by default) that allows
turning off all commands that are trying to reach out to
remotes. Currently, that is only the fetch command which is executed
when trying to determine "ahead" and "behind". Any future operation
that is accessing remote repos should also check this flag.
@TheSnoozer
Copy link
Collaborator

Hi, thanks for your contribution!

I'd agree that this fetch is certainly not the taste of everyone and thus was highlighted in the release notes.

Does the exclusion of the ahead and behind properties still trigger a fetch?
E.g.

<excludeProperties><excludeProperty>^git.local.branch.*$</excludeProperty></excludeProperties>

?

@hgschmie
Copy link
Contributor Author

Looking through the code, it seems that the maybePut in GitDataProvider would not trigger the memoizer for fetching the remote branches if the ahead and behind branches are not accessed. Now, given that this uses the slightly less than obvious syntax of excluding git.local.branch.*$ (so it excludes the local branches to not trigger accessing remote branches???), I still feel it would be justified to have an actual switch to the configuration of the plugin.

This switch would also allow to be triggered in a profile to turn remote access on and off (I use that in connection with properties in the basepom project all the time).

I also updated the PR to respect the maven "-o" offline switch to put maven in offline mode.

@TheSnoozer
Copy link
Collaborator

Hi, thanks for your input.
AFAIK with that exclusion the plugin should also not trigger a pull and thus was mainly raising the question if this plugin really needs yet another switch (considering the many's that are already there).
However I think you really point out the problem with that slightly less obvious syntax and just referencing to an older release note (https://github.com/git-commit-id/maven-git-commit-id-plugin/releases/tag/v3.0.0) where I mentioned this could easily be overlooked. Considering the update to respect the maven "-o" offline makes it even more obvious that there is a need for such new setting.

* The Maven settings.
*/
@Parameter(property = "settings", required = true, readonly = true)
Settings settings;
Copy link
Collaborator

@TheSnoozer TheSnoozer Jul 16, 2019

Choose a reason for hiding this comment

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

This new settings sadly makes the tests fail.

Inside the GitIntegrationTest you also would need to mock them:

  private static Settings mockSettings() {
    Settings settings = mock(Settings.class);
    when(settings.isOffline()).thenReturn(false);
    return settings;
  }

Could you perhaps add this mock? I think besides that this should be good to be merged.

@hgschmie
Copy link
Contributor Author

Hey, sorry for breaking the tests. I added the change to the code; it now builds on my laptop (and also on the CI).

@TheSnoozer
Copy link
Collaborator

Perfect! Thank you for your contribution and your patience to address my comments ;-)

I'm going ahead and merge. This feature will become available in the next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants