-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Accept Gradle build scan agreement #30645
Conversation
Scans will be produced only when passing `--scan`
Pinging @elastic/es-core-infra |
build.gradle
Outdated
} | ||
buildScan { | ||
termsOfServiceUrl = 'https://gradle.com/terms-of-service' | ||
termsOfServiceAgree = 'yes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can speak for everyone who forks the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate Gradle didn't implement a property for this, but I just realized that we can do something like project.properties.get('org.elasticsearch.scanTermsOfServiceAgree', 'no')
. Then one can add org.elasticsearch.scanTermsOfServiceAgree=yes
to ~/.gradle/gradle.properties
to change that default. What do you think of that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure we need to do this at all - the instructions for newer versions of gradle don't mention this code and I'm not really sure what having -scan
work for everyone buys us. But if we feel the need to do this then I figure we could use a system property. So if you run ./gradlew -DscanTOS=agree
then we set up scan. If you don't specify it, then we don't.
I'd honestly lean towards not doing this at all though. I think maybe if folks need to scan they can add it themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one concern.
build.gradle
Outdated
@@ -35,6 +35,13 @@ import org.gradle.util.DistributionLocator | |||
import java.nio.file.Files | |||
import java.nio.file.Path | |||
import java.security.MessageDigest | |||
plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please add a newline between here and the imports
build.gradle
Outdated
plugins { | ||
id 'com.gradle.build-scan' version '1.13.2' | ||
} | ||
buildScan { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should wrap this in a property check so that the default is to not add this. I don't think we should access the terms of service for anyone (ie non elastic employees) running our build. We (developers) can then set this property in our ~/.gradle/gradle.properties
.
@nik9000 I implemented the property based solution. The value is that you can generate scans without having to type at the console e.x. when redirecting output or with CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think we should make the property a simple boolean value.
build.gradle
Outdated
plugins { | ||
id 'com.gradle.build-scan' version '1.13.2' | ||
} | ||
if (properties.get("org.elasticsearch.scanTOS", "no") == "agree") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should yes true/false here, not "no" and "agree".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjernst done
I'm fine with the property based solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* master: QA: Add xpack tests to rolling upgrade (#30795) Modify state of VerifyRepositoryResponse for bwc (#30762) Reduce CLI scripts to one-liners on Windows (#30772) Simplify number of shards setting (#30783) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) [Docs] Fix script-fields snippet execution (#30693) Upgrade to Lucene-7.4.0-snapshot-cc2ee23050 (#30778) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) Make http pipelining support mandatory (#30695) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) Increase the maximum number of filters that may be in the cache. (#30655) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Add more yaml tests for get alias API (#29513) Ignore empty completion input (#30713) [DOCS] fixed incorrect default [ML] Filter undefined job groups from update calendar actions (#30757) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Enable installing plugins from snapshots.elastic.co (#30765) Remove fedora 26, add 28 (#30683) Accept Gradle build scan agreement (#30645) Remove logging from elasticsearch-nio jar (#30761) Add Delete Repository High Level REST API (#30666)
* Accept Gradle build scan argreement Scans will be produced only when passing `--scan` * Condition TOS acceptance with property * Switch to boolean flags
* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)
Scans will be produced only when passing
--scan
on the command line.https://docs.gradle.com/build-scan-plugin/