Reduce flakiness of license fetcher. #484
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
allisonbm92
force-pushed
the
allison-licenses
branch
from
2a3f010 to
2ecbdc4
Compare
yifanyang
reviewed
Jun 3, 2019
buildSrc/src/main/groovy/com/google/firebase/gradle/plugins/license/RemoteLicenseFetcher.groovy
Show resolved
Hide resolved
yifanyang
reviewed
Jun 3, 2019
buildSrc/src/main/groovy/com/google/firebase/gradle/plugins/license/RemoteLicenseFetcher.groovy
Show resolved
Hide resolved
Corrob
reviewed
Jun 3, 2019
| String get() { | ||
| APACHE_2_LICENSE_URI.toURL().getText() | ||
| String processDocument(Document doc) { | ||
| // TODO(vkryachko, allisonbm92): This fails silently. |
There was a problem hiding this comment.
yeah, why do you return an empty string?
There was a problem hiding this comment.
Yes, I've expanded details in the comments and removed the empty string.
|
|
||
| TEXT_FORMATTER.getPlainText(doc.select('body > table > tbody > tr:nth-child(2) > td:nth-child(2) > table > tbody > tr:nth-child(3) > td > en > blockquote')) | ||
| String processDocument(Document doc) { | ||
| return TEXT_FORMATTER.getPlainText(doc.select("body > table > tbody > tr:nth-child(2) > td:nth-child(2) > table > tbody > tr:nth-child(3) > td > en > blockquote").get(0)) |
| @Override | ||
| URI getServiceUri() { | ||
| GNU_CLASSPATH_LICENSE_URI | ||
| // TODO(allisonbm92, vkryachko): This does not fetch the actual license! |
There was a problem hiding this comment.
Can you put the missing work in this TODO? Why doesn't it fetch the actual license?
This commit is a simple fix to reduce the flakiness of downloading licenses. The fetcher will now try up to three times and will insert a small delay between each attempt. This delay will only affect builds that would otherwise fail.
allisonbm92
force-pushed
the
allison-licenses
branch
from
2ecbdc4 to
9cae897
Compare
allisonbm92
added a commit
that referenced
this pull request
Jun 11, 2019
My fix in #484 changed the way the Apache license was handled, and all whitespace was stripped from this popular license. Essentially, the original code fetched it directly with the URL class while the other licenses were fetched with Jsoup. This commit restores this behavior. This has the unfortunate side-effect of re-introducing boilerplate code, but it's the most cost-effective solution for now.
allisonbm92
added a commit
that referenced
this pull request
Jun 11, 2019
My fix in #484 changed the way the Apache license was handled, and all whitespace was stripped from this popular license. Essentially, the original code fetched it directly with the URL class while the other licenses were fetched with Jsoup. This commit restores this behavior. This has the unfortunate side-effect of re-introducing boilerplate code, but it's the most cost-effective solution for now.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is a simple fix to reduce the flakiness of downloading
licenses. The fetcher will now try up to three times and will insert a
small delay between each attempt. This delay will only affect builds
that would otherwise fail.