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-70331: Always honor useExistingAccountWithSameEmail #1382

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

Conversation

reddwarf69
Copy link

@reddwarf69 reddwarf69 commented Dec 23, 2022

useExistingAccountWithSameEmail makes sense on its own, independently of how a new account may be created if required.

JENKINS-70331 - Always honor useExistingAccountWithSameEmail

In general, I'm not convinced about the whole separation in

if (createAccountBasedOnEmail) {
based on createAccountBasedOnEmail. I think there are two independent things:

  • Search
  • Creation if not found
    And "createAccountBasedOnEmail" whould only affect "Creation if not found".

By having the two big branches the "Search" gets duplicated. I'm not trying to fix this here, but I am adding to one of the branches logic that is already in the other one and should IMHO have been in both.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

I don't think there is any doc change strictly required for this PR. But the docs regarding all this could, in general, be improved.

Types of changes

  • [ ? ] Bug fix (non-breaking change which fixes an issue)
  • [ ? ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

I guess this could potentially break for somebody? Not exactly sure how.

useExistingAccountWithSameEmail makes sense on its own, independently of
how a new account may be created if required.
@github-actions github-actions bot added the test label Dec 23, 2022
@reddwarf69
Copy link
Author

My understanding is that the only test failing is https://github.com/jenkinsci/git-plugin/pull/1382/checks?check_run_id=10352017446, and that it is unrelated to this change. Am I right?

@MarkEWaite
Copy link
Contributor

My understanding is that the only test failing is https://github.com/jenkinsci/git-plugin/pull/1382/checks?check_run_id=10352017446, and that it is unrelated to this change. Am I right?

Yes, that is correct. When tests run in parallel (as they do on ci.jenkins.io) there may be times when two tests are making a change to the git global configuration. Command line git locks the global configuration file (as it should) when it is being modified. I'll run the job again after my current tasks preparing the git plugin 5.0.0 release are complete.

Git client plugin 4.0.0 has released. Git plugin 5.0.0 release process is running now.

@MarkEWaite MarkEWaite added tests Automated test addition or improvement and removed test labels Sep 22, 2023
@MarkEWaite MarkEWaite added enhancement Improvement or new feature and removed tests Automated test addition or improvement labels Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature
Projects
None yet
2 participants