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

Use email-aliases-plugin to find authors User objects #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

i386
Copy link
Contributor

@i386 i386 commented Nov 12, 2016

Users often have multiple email addresses that they use to commit changes in Git. It is not always the case that a users committer email and Jenkins email will be the same.

email-aliases-plugin allows a User to defined a set of email addresses that are related to their identity. When we run autofavorite we should use these addresses to help more accurately discover the user to assign the favorite to.

This PR depends on the initial commit of the email-aliases-plugin so it should be reviewed in conjunction with this PR. See https://github.com/i386/email-aliases-plugin/pull/1.

if (User.getById(author.getId(), false) == null || !((GitSCM) scm).isCreateAccountBasedOnEmail()) {
UserProperty property = first.getAuthor().getProperty(UserProperty.class);
if (property != null && property.getConfiguredAddress() != null) {
for (User user : User.getAll()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linear search feels wrong here for a few reasons:

  1. log(n) time to search here should not be added to the build time of the first run. Probably should offload this to a different thread?
  2. I suspect it is expensive to load all Users like this but I cant be sure.

Advice wanted :)

final Job<?, ?> job = build.getParent();
User author = first.getAuthor();

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting changes from this point onward.

@i386
Copy link
Contributor Author

i386 commented Nov 12, 2016

@daniel-beck @imeredith @vivek it would be great to get a review here.

@i386 i386 force-pushed the topic/email-alias branch from 48429ce to 21f3f9c Compare November 13, 2016 00:00

@Nullable
private User findUserByEmailAlias(@Nonnull String email) {
for (User user : User.getAll()) {
Copy link
Contributor Author

@i386 i386 Nov 13, 2016

Choose a reason for hiding this comment

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

I realise this is bad and I feel bad. I am considering creating a Search API in the email-aliases plugin to do this efficiently for multiple callers.

@i386 i386 force-pushed the topic/email-alias branch from 21f3f9c to 9fa1978 Compare November 13, 2016 00:20
@halkeye
Copy link
Member

halkeye commented Apr 1, 2019

I know this is kinda moot now, but i always used https://wiki.jenkins.io/display/JENKINS/Additional+Identities+Plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants