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

Accept Gradle build scan agreement #30645

Merged
merged 4 commits into from
May 22, 2018
Merged

Accept Gradle build scan agreement #30645

merged 4 commits into from
May 22, 2018

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented May 16, 2018

Scans will be produced only when passing --scan on the command line.

https://docs.gradle.com/build-scan-plugin/

Scans will be produced only when passing
`--scan`
@alpar-t alpar-t added >non-issue :Delivery/Build Build or test infrastructure labels May 16, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

build.gradle Outdated
}
buildScan {
termsOfServiceUrl = 'https://gradle.com/terms-of-service'
termsOfServiceAgree = 'yes'
Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Member

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?

Copy link
Member

@rjernst rjernst left a 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 {
Copy link
Member

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 {
Copy link
Member

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.

@rjernst
Copy link
Member

rjernst commented May 17, 2018

Oops, I forgot to push the button on my comments yesterday and just now pushed them. @nik9000 if you don't have this in build.gradle, you get a prompt at the very end of the build to accept the TOS. I think doing this with a property (as @atorok suggests) is the way to go.

@alpar-t
Copy link
Contributor Author

alpar-t commented May 18, 2018

@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.

Copy link
Member

@rjernst rjernst left a 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") {
Copy link
Member

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjernst done

@nik9000
Copy link
Member

nik9000 commented May 18, 2018

I'm fine with the property based solution.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@alpar-t alpar-t merged commit ff62638 into elastic:master May 22, 2018
@alpar-t alpar-t deleted the build/scan branch May 22, 2018 04:21
dnhatn added a commit that referenced this pull request May 22, 2018
* 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)
alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Aug 27, 2018
* Accept Gradle build scan argreement

Scans will be produced only when passing
`--scan`

* Condition TOS acceptance with property

* Switch to boolean flags
jasontedor added a commit that referenced this pull request Aug 27, 2018
* 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)
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants