Get branch name from env var (#74) #76

alextu · 2022-07-15T12:36:27Z

The implemented behavior will try to get the branch name from the env var first (even if the Git CLI is available).
This PR includes some refactoring to rely on Optional.

runningcode

looks good, i have mostly comments around the GitMetadata class but I can't think of the best way to improve it in my head.

runningcode · 2022-07-18T12:13:44Z

src/main/java/com/gradle/CustomBuildScanEnhancements.java

-            if (isNotEmpty(gitRepo) && isNotEmpty(gitCommitId)) {
-                if (gitRepo.contains("github.com/") || gitRepo.contains("github.com:")) {
-                    Matcher matcher = Pattern.compile("(.*)github\\.com[/|:](.*)").matcher(gitRepo);
+            if (gitRepo.isPresent() && gitCommitId.isPresent()) {


the previous logic would also ensure the resolved value is not empty, could we also check that here?

I could but the current code implicitly already handles the empty strings in the nested ifs

I've hardened this by making sure having an empty string does not happen (see GitMetadataResolver logic and test)

src/main/java/com/gradle/GitMetadata.java

runningcode · 2022-07-18T12:19:03Z

src/main/java/com/gradle/GitMetadata.java

+        this.isGitInstalled = isGitInstalled;
+    }
+
+    static GitMetadata given(boolean isGitInstalled) {


i'm not a fan of the name given here for the static constructor method since it isn't very descriptive when used inline.

it's been removed with the builder changes

runningcode · 2022-07-18T12:20:28Z

src/main/java/com/gradle/GitMetadata.java

+
+import static com.gradle.Utils.execAndGetStdOut;
+
+class GitMetadata {


I like having a class here encapsulate the logic for fetching the gitmetadata but it seems to me that the encapsulation is not fully complete and requires the caller to know the implementation details. for example you must know you need to pass either the cmd args or the value supplier depending on whether git is installed or not.

That's a good point, actually I made a change to be more flexible and silently accept the case: git is installed but no cmd is provided.

runningcode · 2022-07-25T15:25:03Z

src/main/java/com/gradle/GitMetadata.java

+        return Optional.empty();
+    }
+
+    static class GitMetadataBuilder {


in the current state, this feels a bit more like a Factory to me :D

I'm trying to think of the best way to structure this though. i haven't really fully thought it out but the current implementation seems like it could be simplified.

yeah, i think a factory would make sense since it would remove the case of the .build().resolve() that is used in every case.

How about the latest refactoring a bit less expressive but simple: a resolver that just resolves :))

runningcode

Looks good to me aside from the testing change.

We have intentionally kept this repository free of tests in order to make it as simple as possible for customers to copy and understand. I am not saying we should keep or remove the tests but I think it needs to be discussed before making a change like this to add these.

@etiennestuder also wants to take a look at this so I'll let him have the final review.

etiennestuder · 2023-01-03T06:45:34Z

I would hold off on merging this PR until we have resolved the config caching issue explored in this PR and raised in this issue.

I believe we need to make significant changes to where we read the env vars in order to not cause constant config cache invalidation. Depending on how that will be solved, it might require a different approach/implementation.

runningcode · 2023-01-31T13:00:32Z

Closing PR for now. Let's re-open after making the CC compatibility changes.

erichaagdev · 2023-02-28T19:16:27Z

src/main/java/com/gradle/CustomBuildScanEnhancements.java

@@ -381,24 +369,15 @@ private boolean isGitInstalled() {
            return execAndCheckSuccess("git", "--version");
        }

-        private String getGitBranchName(Supplier<String> gitCommand) {
+        private Optional<String> getGitBranchNameFromEnv() {


Why is GitHub actions not included?

etiennestuder · 2023-03-01T07:40:10Z

@erichaagdev how often does this actually come up? We have 1 case for sure which is the one that triggeed this PR. Before time is spent on this PR, we need to be certain is worth the added complexity to itegrate and maintain. 1 single customer would not justify it.

runningcode · 2023-03-01T07:49:15Z

I don't think it was intentionally left out.
I think we should also include GitHub actions but this PR is very, very old. This would improve our samples and our builds. For example here

erichaagdev · 2023-03-01T15:01:22Z

@etiennestuder This would address the issue where CI checks out the branch in a detached HEAD state. In this case,
git rev-parse --abbrev-ref HEAD returns HEAD rather than the branch name.

This is the case for GitHub Actions when a workflow is triggered by a pull_request event. For example, consider this build for android-cache-fix-gradle-plugin:

GitHub Actions checks out the branch in a detached HEAD state: https://github.com/gradle/android-cache-fix-gradle-plugin/actions/runs/4299768295/jobs/7495318117#step:2:372
The corresponding Build Scan is tagged with HEAD and has a custom value of Git branch=HEAD: https://ge.solutions-team.gradle.com/s/5kt2y4de3ahka/custom-values#L2

My expectation in this case is that the Build Scan is tagged with erichaagdev/update-daily-dependabot-time and has a custom value of Git branch=erichaagdev/update-daily-dependabot-time.

Of course this has the side effect that you cannot filter the Build Scan dashboard to include only builds for your branch.

To better understand the impact this has, consider this filter project: android-cache-fix-gradle-plugin and tags: CI not:main not:head for the last 90 days. There are only 16 builds shown: https://ge.solutions-team.gradle.com/scans?search.relativeStartTime=P90D&search.rootProjectNames=android-cache-fix-gradle-plugin&search.tags=ci%2Cnot:head%2Cnot:main&search.timeZoneId=America/Chicago

Now if we drop the tag: HEAD, we have 1954 builds: https://ge.solutions-team.gradle.com/scans?search.relativeStartTime=P90D&search.rootProjectNames=android-cache-fix-gradle-plugin&search.tags=ci,not:main&search.timeZoneId=America/Chicago

This PR also defines a pattern for how to address #114. In that case, builds running on Bamboo with the repository caching feature enabled will see the Git repository custom value as a local file path, rather than the remote repository. See the issue for more details.

etiennestuder · 2023-03-01T16:49:56Z

@erichaagdev Thanks for the details. But you are not addressing the core of my question which is: how often does this come up with customers?

erichaagdev · 2023-03-01T17:05:46Z

Sorry, @etiennestuder but I will not be able to provide an exact frequency.

This would be a problem for any customer using GitHub Actions to run workflows on pull_request events, or any other CI provider which checks out projects in a detached state.

The similar #114 issue that I referenced is actually from a customer.

runningcode · 2024-01-08T11:08:16Z

Closing in favor of #240

alextu requested a review from runningcode July 15, 2022 12:38

runningcode reviewed Jul 18, 2022

View reviewed changes

alextu force-pushed the atual/fix-branch branch 3 times, most recently from a4ea7f7 to 4b70d9d Compare July 25, 2022 15:09

alextu requested a review from runningcode July 25, 2022 15:10

runningcode reviewed Jul 25, 2022

View reviewed changes

alextu force-pushed the atual/fix-branch branch from 9024531 to c4b8632 Compare July 26, 2022 13:48

Get branch name from env var (#74)

fc139f1

alextu force-pushed the atual/fix-branch branch from c4b8632 to fc139f1 Compare July 26, 2022 15:24

alextu requested a review from runningcode July 26, 2022 15:28

runningcode reviewed Jul 26, 2022

View reviewed changes

etiennestuder and others added 3 commits November 19, 2022 05:26

Merge branch 'main' into atual/fix-branch

f9a81ed

Merge branch 'main' into atual/fix-branch

2552fd1

Merge branch 'main' into atual/fix-branch

91b0c46

erichaagdev approved these changes Dec 28, 2022

View reviewed changes

erichaagdev reviewed Feb 28, 2023

View reviewed changes

runningcode closed this Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get branch name from env var (#74) #76

Get branch name from env var (#74) #76

alextu commented Jul 15, 2022

runningcode left a comment

runningcode Jul 18, 2022

alextu Jul 18, 2022

alextu Jul 26, 2022

runningcode Jul 18, 2022

alextu Jul 18, 2022

runningcode Jul 18, 2022

alextu Jul 18, 2022

runningcode Jul 25, 2022

runningcode Jul 25, 2022

alextu Jul 25, 2022

runningcode left a comment

etiennestuder commented Jan 3, 2023

runningcode commented Jan 31, 2023

erichaagdev Feb 28, 2023

etiennestuder commented Mar 1, 2023 •

edited

runningcode commented Mar 1, 2023

erichaagdev commented Mar 1, 2023

etiennestuder commented Mar 1, 2023

erichaagdev commented Mar 1, 2023

runningcode commented Jan 8, 2024


		import static com.gradle.Utils.execAndGetStdOut;

		class GitMetadata {

Get branch name from env var (#74) #76

Get branch name from env var (#74) #76

Conversation

alextu commented Jul 15, 2022

runningcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runningcode left a comment

Choose a reason for hiding this comment

etiennestuder commented Jan 3, 2023

runningcode commented Jan 31, 2023

Choose a reason for hiding this comment

etiennestuder commented Mar 1, 2023 • edited

runningcode commented Mar 1, 2023

erichaagdev commented Mar 1, 2023

etiennestuder commented Mar 1, 2023

erichaagdev commented Mar 1, 2023

runningcode commented Jan 8, 2024

etiennestuder commented Mar 1, 2023 •

edited