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

[SPARK-26918][DOCS] All .md should have ASF license header #24243

Closed
wants to merge 2 commits into from

Conversation

srowen
Copy link
Member

@srowen srowen commented Mar 29, 2019

What changes were proposed in this pull request?

Add AL2 license to metadata of all .md files.
This seemed to be the tidiest way as it will get ignored by .md renderers and other tools. Attempts to write them as markdown comments revealed that there is no such standard thing.

How was this patch tested?

Doc build

@srowen srowen self-assigned this Mar 29, 2019
@srowen srowen requested a review from felixcheung March 29, 2019 19:00
@SparkQA
Copy link

SparkQA commented Mar 29, 2019

Test build #104091 has finished for PR 24243 at commit b841b39.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

thx!!

Copy link
Contributor

@attilapiros attilapiros left a comment

Choose a reason for hiding this comment

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

Question about including the license twice otherwise LGTM.

I have checked for including double licenses by executing the following search and there is only ./docs/cloud-integration.md where this problem occurs:

$ find . -name "*.md" -exec grep -c "http://www.apache.org/licenses" \{} \; -print | grep 2 -A 2
2
./docs/cloud-integration.md
1
--
./build/scala-2.12.7/doc/LICENSE.md
2
./build/scala-2.12.8/doc/LICENSE.md
1
./build/scala-2.11.7/doc/LICENSE.md
1
./build/scala-2.11.8/doc/LICENSE.md
1
./build/scala-2.11.12/doc/LICENSE.md
0
0

@attilapiros
Copy link
Contributor

I run a completness check too. As I understand only the markdowns under the ./docs are targeted, in this case the PR is also complete:

$ find . -name "*.md" -exec grep -L "http://www.apache.org/licenses" \{} \;
./resource-managers/kubernetes/integration-tests/README.md
./core/target/scala-2.12/classes/org/apache/spark/deploy/security/README.md
./core/src/main/scala/org/apache/spark/deploy/security/README.md
./python/README.md
./R/WINDOWS.md
./R/README.md
./R/DOCUMENTATION.md
./R/CRAN_RELEASE.md
./README.md
./common/network-common/src/main/java/org/apache/spark/network/crypto/README.md
./common/tags/README.md
./CONTRIBUTING.md
./dev/appveyor-guide.md
./dev/README.md
./external/docker/README.md
./external/docker/spark-test/README.md
./sql/core/target/scala-2.12/classes/org/apache/spark/sql/test/README.md
./sql/core/src/test/README.md
./sql/core/src/main/scala/org/apache/spark/sql/test/README.md
./sql/README.md

@srowen
Copy link
Member Author

srowen commented Mar 30, 2019

I'll fix all that, yeah. The only annoying thing is that this metadata gets displayed in Github like ...
https://github.com/srowen/spark/blob/SPARK-26918/docs/building-spark.md
... with a big blob at the top. For README.md files clearly meant for consumption on Github, that's distracting. I wonder if we can plausibly not do this for the very top-level project one given how the license is already linked twice by Github when you're looking at that page.

@attilapiros
Copy link
Contributor

Regarding github markdown comment I have found this working:

[//]: comment

But unfortunately it should be combined with the metadata and that I am afraid won't work.
I have not found any multi-line comments which kinda makes sense for markdown (to keep parsing simple).

@srowen
Copy link
Member Author

srowen commented Mar 30, 2019

I tried that and it rendered in the HTML output, so abandoned it. I couldn't find something that works with jekyll other than metadata

@attilapiros
Copy link
Contributor

I have only one idea: to introduce an extra "build" step where you filter out lines starting with [//]: and pass the result to jekyll.

@SparkQA
Copy link

SparkQA commented Mar 30, 2019

Test build #104106 has finished for PR 24243 at commit 226a238.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member Author

srowen commented Mar 30, 2019

Yeah... I figure it's not worth it unless we find for some reason this solution isn't sufficient or it's too annoying. I like that it's metadata in this case. If we had to, it could convert to HTML comments but that puts the bulk of the comment in the HTML too.

@felixcheung
Copy link
Member

felixcheung commented Mar 30, 2019 via email

@srowen
Copy link
Member Author

srowen commented Mar 31, 2019

Merged to master

@srowen srowen closed this in 754f820 Mar 31, 2019
@srowen srowen deleted the SPARK-26918 branch March 31, 2019 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants