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

Start using Gradle Enterprise instance #4663

Merged
merged 8 commits into from
Nov 21, 2021
Merged

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Nov 17, 2021

No description provided.

@@ -28,13 +29,13 @@ dependencyResolutionManagement {
val isCI = System.getenv("CI") != null
val skipBuildscan = System.getenv("SKIP_BUILDSCAN").toBoolean()
gradleEnterprise {
server = "https://ge.opentelemetry.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously scan was published only when building from ci but this check is now removed. Does this mean that scan is uploaded even for local builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is Gradle's suggestion, yes. The goal is to better understand builds (especially build failures) on local machines. If we are concerned about privacy we may change that to publish scans only on failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's too surprising if OSS builds phone home without an opt-in. Eventually someone will notice and complain hard :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will change to publish only on CI or if opted in

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

🥳

(+1 to @laurit's question above)

id("com.github.burrunan.s3-build-cache") version "1.2"
id("com.gradle.common-custom-user-data-gradle-plugin") version "1.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/gradle/gradle-enterprise-build-config-samples/tree/master/common-custom-user-data-gradle-plugin

A tag representing the operating system
A tag representing how the build was invoked, be that from your IDE (IDEA, Eclipse, Android Studio) or from the command-line
A tag representing builds run on CI, together with a set of tags, links and custom values specific to the CI server running the build
For Git repositories, information on the commit id, branch name, status, and whether the checkout is dirty or not

I'm not sure what "tag representing OS" means here, but it seems a bit scary for an OSS project, naturally we will be publishing scans from local corp laptops sometimes to troubleshoot builds. This plugin smells like it's intended for corporate, not OSS use.

Can we leave it out for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I will also ask Gradle people to comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anuraaga As publish from local machine now require opt-in, do you still want to remove this plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably OK (I'm guessing OS tag is just "mac" type of string)

Choose a reason for hiding this comment

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

It will add Mac OS X to your builds (amongst other tags). it is very useful to debug issues to know if a build came from Mac or windows for example because line endings or encoding may cause issues.
Here is an example of what that tagging looks like on the JUnit Gradle Enterprise OSS instance: https://ge.junit.org/s/katto6gtipkwe#info

Choose a reason for hiding this comment

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

Suggested change
id("com.gradle.common-custom-user-data-gradle-plugin") version "1.2.1"
id("com.gradle.common-custom-user-data-gradle-plugin") version "1.5"

id("com.github.burrunan.s3-build-cache") version "1.2"
id("com.gradle.common-custom-user-data-gradle-plugin") version "1.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably OK (I'm guessing OS tag is just "mac" type of string)

buildScan {
termsOfServiceUrl = "https://gradle.com/terms-of-service"
termsOfServiceAgree = "yes"
publishAlwaysIf(isCI || publishBuildscan)

Choose a reason for hiding this comment

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

Suggested change
publishAlwaysIf(isCI || publishBuildscan)
publishAlways()
publishIfAuthenticated()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@runningcode Do we need both always and ifAuthenticated?

Choose a reason for hiding this comment

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

Yes, its undocumented but this is the recommended set up for OSS projects.

@trask
Copy link
Member

trask commented Nov 19, 2021

@iNikem is this expected prior to merging this PR?

A build scan was not published as you have not authenticated with server 'ge.opentelemetry.io'.

@iNikem
Copy link
Contributor Author

iNikem commented Nov 20, 2021

@iNikem is this expected prior to merging this PR?

A build scan was not published as you have not authenticated with server 'ge.opentelemetry.io'.

Unfortunately. As our GE instance is public, it is recommended (by Gradle folks and, probably, common sense) to allow only authenticated users to publish build scans. This means only builds initiated by our repo (and not PRs from forks, because GH secrets are not passed to them) and maintainers who wish to configure authentication for their local machines. This limits the benefits of GE, but we don't know a way around that.

We could continue to publish build scan for all other builds to scans.gradle.org as we did before, but as it was pointed out to me this means that we accept Gradle scans' TOS in the name of anybody who happens to run a build/create a PR. Which is not legally right thing to do. But maybe doing so only for PRs is OK?

@trask
Copy link
Member

trask commented Nov 20, 2021

Unfortunately. As our GE instance is public, it is recommended (by Gradle folks and, probably, common sense) to allow only authenticated users to publish build scans. This means only builds initiated by our repo (and not PRs from forks, because GH secrets are not passed to them) and maintainers who wish to configure authentication for their local machines. This limits the benefits of GE, but we don't know a way around that.

got it. I think that's ok since all of the non-PR builds will still give us plenty of builds to track flaky tests across. and we'll still get the gradle cache benefits for PRs and local builds.

We could continue to publish build scan for all other builds to scans.gradle.org as we did before, but as it was pointed out to me this means that we accept Gradle scans' TOS in the name of anybody who happens to run a build/create a PR. Which is not legally right thing to do. But maybe doing so only for PRs is OK?

this is a good question.

we currently recommend reviewing the gradle scan for troubleshooting PR build failures: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/CONTRIBUTING.md#troubleshooting-pr-build-failures

for this PR, can you keep publishing PR build scans to scans.gradle.org, since that was prior behavior, and send a separate PR to remove it (and to remove the above doc)? then we can request CNCF legal review, and bring it back depending on their guidance

@iNikem
Copy link
Contributor Author

iNikem commented Nov 21, 2021

Current state:

  • If GE authentication present, then publish to ge.opentelemetry.io
  • if GE authentication is not present and we are in CI, then publish to scans.gradle.com (will be removed in future)
  • if GE cache username is present and we are in CI, then push to remote GE cache

@iNikem iNikem merged commit 254a267 into open-telemetry:main Nov 21, 2021
@iNikem iNikem deleted the ge branch November 21, 2021 16:53
@anuraaga
Copy link
Contributor

Oh I didn't know this would only work with explicit opt-in via adding credentials. In that case we can have the scan published automatically if you'd like, it's not implicit phone home like I thought before.

@iNikem
Copy link
Contributor Author

iNikem commented Nov 22, 2021

My initial version was "publish always" :) But turned out, it could not work :) So yes, only if authenticated.

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Start using Gradle Enterprise instance

* Require opt-in to publish build scan from local machine

* Publish build scan only if authenticated

* Switch to GE cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants