[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721758994 Thanks! 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721756082 I see. Thanks a lot for your patience and the explanations. 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721712709 I rebased the change to master, locally the license check has passed. Once you give approval, I'll clean up the history, rebase & run it on CI one more time and then 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721710429 Ah, sorry. My comments have been confusing. I've initially had a scope there, then removed it (probably even before opening the PR, that's why I assumed you are referring to an outdated version of the PR). Isn't the "compile" scope the default scope: http://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope The dependencyManagement section of flink-parent isn't changing the scope of the dependency. 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721684343 I've addressed this comment a while ago. Maybe you are looking at old commits? 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-721611660 Thanks a lot for your review. I addressed all outstanding comments. Once you declare the changes so far to be good to be merged, I'll rebase to master. We might have additional licensing issues to address then .. let's see 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-720939621 Thanks a lot for your feedback. I pushed a commit addressing your comments. 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-718145230 > There is no benefit in having the generated files in git, it could just be step of the release process One benefit would be, that our release process is less complicated Second benefit is that it is easier to check that the generated NOTICE files are valid when you have to regenerate and commit them. With your approach, we would wait with that until the release, and it would only be done by a few "experts". I'm undecided here, just discussing to understand our options. I'll soon push an update that resolves all errors we have so far in our notice files. 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-717967845 How would you integrate such a notice file generating build step into the CI pipeline? - I've quickly looked at building our own shade resource transformer, but it seems that we won't have the necessary information available. We would probably need to go even further and change more stuff in the shade plugin. - The current approach depends on the maven build output. We could write the NOTICE file changes directly when the tool runs. Similar to the documentation generator, contributors would need to run the tool locally to keep the NOTICE files up to date. I guess this approach would be preferred. Any other ideas? 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-717859165 Once I'm done with the license check for this release, I can make an assessment of how well the tool works. If I'm confident that it can also handle edge-cases, I could look into automating it. What we need is basically a mapping from dependency --> license. We could make the build fail if this mapping doesn't exist for a dependency. I will proceed with addressing the licensing issues I've found so far. I was planning to open separate PRs for different areas of Flink (python, table, connectors), or shall I add them all here to this PR? (this would be easier for me, but only works if you are willing to review them all at once) 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: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13796: [FLINK-19810][CI] Automatically run a basic NOTICE file check on CI
rmetzger commented on pull request #13796: URL: https://github.com/apache/flink/pull/13796#issuecomment-717761562 Thanks a lot for taking a look. I believe the statement by the tool is correct: The notice file contains the following dependency: `com.apache.commons:commons-compress:1.20`, but the expected dependency is `org.apache.commons:commons-compress:1.20`. Notice the `com.` vs `org.` Do you agree to how the tool is integrated into the build process in priciple? If so, I will check and fix all dependency issues found by the tool, so that we can 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: us...@infra.apache.org
