You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/04/28 03:42:00 UTC
[GitHub] [beam] Hannah-Jiang opened a new pull request #11548: [BEAM-9136] Introduce no-licenses tag
Hannah-Jiang opened a new pull request #11548:
URL: https://github.com/apache/beam/pull/11548
`no-licenses` tag is used to skip license related tasks when create docker images with Java and Python.
R: @tvalentyn
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416976074
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
There are some more discussion at the thread today, in case you haven't seen it yet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r417739219
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
Do you have other comments? If not, I am going to merge it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-620990823
@tvalentyn, I changed it to pull licenses only with Jenkins test. License pulling is skipped by default. Can you please take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r418160747
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
This is my other comment(copying from above, in case you missed it): I suggested on mailing list that we can also pull licences whenever we run build containers on the release branch. If community does not like that option for some reason, then we should also update the release guide in this PR to make sure release manager is aware that they need to pass a non-default option, and check that licenses were added successfully.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r417654992
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
Wording suggestion:
// Include dependency licences when we build docker images on Jenkins, see: https://s.apache.org/zt68q.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-622059726
Thank you Valentyn.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r417735845
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
I will change the comment with different PR, because this PR triggers 10 tests and takes time to run and some tests are flaky.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-620977492
Run PythonDocker PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-620974622
Run seed job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416962452
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Right, so to produce lightweight images we can skip license download. Why would we ever want to 'check' urls but not downloading the licenses? From your comment:
```
I tried to check if urls are valid instead of pulling the files and it
reduced only 1 min of running time. So, it's not an option here.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Introduce no-licenses tag
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416735375
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Currently, there are three pull licenses options for Java.
when docker-pull-licenses is set, licenses are pulled.
when no-licenses is set, all license pulling related tasks are skipped.
when no tag is passed, which is default mode, license urls are checked.
With Python, there are two options available.
with no-licenses tag, license pulling is skipped.
when no tag is passed, which is default, licenses are pulled.
Python doesn't support checking urls at the moment.
I agree above settings are confuse, and would like to simplify it and make it consistent between docker images.
So, we will have only one tag, which is `docker-pull-licenses`.
when docker-pull-licenses = 1 or true, licenses are pulled.
when docker-pull-licenses = 0 or false, license related tasks are skipped.
when docker-pull-licenses = check, license urls are checked.
Default mode is docker-pull-licenses = check both for Java and Python.
We need to update the script that pull licenses for Python to support checking urls.
Does this sound good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Introduce no-licenses tag
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416735375
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Currently, there are three pull licenses options for Java.
when docker-pull-licenses is set, licenses are pulled.
when no-licenses is set, all license pulling related tasks are skipped.
when no tag is passed, which is default mode, license urls are checked.
With Python, there are two options available.
with no-licenses tag, license pulling is skipped.
when no tag is passed, which is default, licenses are pulled.
I agree above settings are confuse, and would like to simplify it and make it consistent between docker images.
So, we will have only one tag, which is docker-pull-licenses.
when docker-pull-licenses = 1 or true, licenses are pulled.
when docker-pull-licenses = 0 or false, license related tasks are skipped.
when docker-pull-licenses = check, license urls are checked.
Default mode is docker-pull-licenses = check both for Java and Python.
We need to update the script that pull licenses for Python to support checking urls.
Does this sound good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r418241059
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
yep, rewording and `isRelease` tag detection for docker image build with be in another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r418171888
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
I saw on the mailing list that you have updated release guide to include the config flags to build images, and will be updating them. You can do those changes in a follow-up pr.
You can conisider looking at the value of
https://github.com/apache/beam/blob/74a6565c8b64d9fadf256370df47a4c5dadafb55/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L296
to determine whether the task is running for release branch, instead of asking release manager to pass a config flag.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r418160747
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
This is my other comment: I suggested on mailing list that we can also pull licences whenever we run build containers on the release branch. If community does not like that option for some reason, then we should also update the release guide in this PR to make sure release manager is aware that they need to pass a non-default option, and check that licenses were added successfully.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r417735845
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
I will change the comment with different PR, because this PR triggers 10 tests takes time to run and some tests are flaky.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416855737
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Sounds good. Sorry if this was covered in the discussion that I didn't read, but why would we want to check urls but not pull? Feel free to point me to the discussion.
Naming suggestion:
licenses-in-docker-images=add
licenses-in-docker-images=skip
licenses-in-docker-images=check_urls
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416975896
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
The purpose of checking urls was to make sure files can be pulled from the urls when create images. If we set it to skip, no url checking process happened for the PRs and we may see many license issues when we create release images. Then release managers need to fix the issues. If we add the checking urls for each PR, contributors should fix the issues if any and will not have to fix them during release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416962452
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Right, so to produce lightweight images we can skip license download. Why would we ever want to 'check' urls but not downloading the licenses? Quoting your comment on the mailing list:
> I tried to check if urls are valid instead of pulling the files and it
> reduced only 1 min of running time. So, it's not an option here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-620977305
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Introduce no-licenses tag
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416735375
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
Currently, there are three pull licenses options for Java.
when docker-pull-licenses is set, licenses are pulled.
when no-licenses is set, all license pulling related tasks are skipped.
when no tag is passed, which is default mode, license urls are checked.
With Python, there are two options available.
with no-licenses tag, license pulling is skipped.
when no tag is passed, which is default, licenses are pulled.
Python doesn't support checking urls at the moment.
I agree above settings are confuse, and would like to simplify it and make it consistent between docker images.
So, we will have only one tag, which is `docker-pull-licenses`.
when docker-pull-licenses = pull, licenses are pulled.
when docker-pull-licenses = skip, license related tasks are skipped.
when docker-pull-licenses = check, license urls are checked.
Default mode is docker-pull-licenses = check both for Java and Python.
We need to update the script that pull licenses for Python to support checking urls.
Does this sound good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #11548: [BEAM-9136] Introduce no-licenses tag
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416621001
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
How about we call both flags 'pull_licenses' and add "pull_licenses=true" to project root, with a comment what this flag does, and how to disable it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r417735845
##########
File path: .test-infra/jenkins/CommonJobProperties.groovy
##########
@@ -163,6 +163,10 @@ class CommonJobProperties {
// For [BEAM-4847], hardcode Xms and Xmx to reasonable values (2g/4g).
context.switches("-Dorg.gradle.jvmargs=-Xms2g")
context.switches("-Dorg.gradle.jvmargs=-Xmx4g")
+
+ // Add docker-pull-licenses option to all Jenkins test to add licenses to when build docker images.
Review comment:
I will change the comment with different PR, because this PR triggers 10 tests and takes time to run and some tests are flaky.
For the comment about detecting isRelease tag, it will also be part of another PR about release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on pull request #11548: [BEAM-9136] Skip pulling licenses by default.
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #11548:
URL: https://github.com/apache/beam/pull/11548#issuecomment-624966273
Seems like this broke cross-language tests.
https://builds.apache.org/view/A-D/view/Beam/view/PostCommit/job/beam_PostCommit_XVR_Flink/
https://scans.gradle.com/s/x24iwckafknka/console-log?task=:sdks:java:container:pullLicenses
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Hannah-Jiang commented on a change in pull request #11548: [BEAM-9136] Improve docker-pull-licenses tag.
Posted by GitBox <gi...@apache.org>.
Hannah-Jiang commented on a change in pull request #11548:
URL: https://github.com/apache/beam/pull/11548#discussion_r416874101
##########
File path: sdks/java/container/build.gradle
##########
@@ -101,16 +84,44 @@ docker {
project.rootProject["docker-tag"] : project.sdk_version)
dockerfile project.file("./${dockerfileName}")
files "./build/"
+ buildArgs(['pull_licenses': !project.rootProject.hasProperty(["no-licenses"])])
Review comment:
In short, users want to create a lightweight images, without adding licenses.
Lightweight images are welcomed for Jenkins test as well, it reduces image size by 85MB for Java image. More discussion can be found at [here](https://lists.apache.org/thread.html/rff9f05e08de6adf7c39c0f4c59f97ae1a2f3602768480fe9e31e0428%40%3Cdev.beam.apache.org%3E)
Naming suggestion sounds good to me, thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org