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

[JENKINS-52639] Remove snakeyaml dependency by switching to wordnet-random-name #147

Merged
merged 2 commits into from Jul 19, 2018

Conversation

@dwnusbaum
Copy link
Member

commented Jul 18, 2018

See JENKINS-52639.

Using snakeyaml without shading it is a little precarious in the current Jenkins ecosystem, see the comments in JENKINS-50202 for details.

This PR replaces the javafaker library with wordnet-random-name. It doesn't allow us to use different themes for different kinds of data (which might be a blocker), but it has no compile-scope dependencies.

We could also downgrade to snakeyaml 1.17 to avoid API compatibility issues for now (assuming javafaker 0.15 is compatible with snakeyaml 1.17), or we could shade it and the javafaker library to avoid conflicts with other plugins, but getting rid of it seemed like the easiest long-term solution.

@reviewbybees

@dwnusbaum dwnusbaum requested a review from jvz Jul 18, 2018

@Vlatombe
Copy link
Member

left a comment

lgtm

@jvz
jvz approved these changes Jul 18, 2018
Copy link
Member

left a comment

LGTM. Too bad about SnakeYAML.

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.Nonnull;
import java.util.Locale;
import java.util.Set;
import org.apache.commons.lang.StringUtils;

This comment has been minimized.

Copy link
@jvz

jvz Jul 18, 2018

Member

Yuck, I like using the current versions of Commons libraries. :)

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Jul 18, 2018

Author Member

Yeah, commons-lang has a better compatibility story than snakeyaml IIRC, so it would likely be fine to add a commons-lang3 dependency, but since commons-lang 2.6 is already bundled in Jenkins core it seems easiest to just use that to avoid any issues.

@@ -52,14 +52,14 @@ public static DataFaker get() {
return ExtensionList.lookupSingleton(DataFaker.class);
}

private final Faker faker = new Faker(Locale.ENGLISH);
private final RandomNameGenerator faker = new RandomNameGenerator();

This comment has been minimized.

Copy link
@varyvol

varyvol Jul 19, 2018

Contributor

A little picky, but the fact that this variable is still called faker was a little confusing to me at the beginning.

@dwnusbaum dwnusbaum requested a review from varyvol Jul 19, 2018

@dwnusbaum dwnusbaum changed the title Remove snakeyaml dependency by switching to wordnet-random-name [JENKINS-52639] Remove snakeyaml dependency by switching to wordnet-random-name Jul 19, 2018

@dwnusbaum dwnusbaum merged commit c39e161 into jenkinsci:master Jul 19, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@dwnusbaum dwnusbaum deleted the dwnusbaum:remove-snakeyaml-dep branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.