Skip to content

Conversation

@dodo920306
Copy link
Contributor

@dodo920306 dodo920306 commented May 17, 2025

Ensure a connection is unique before adding it to the connection list.

To prevent data inconsistency caused by partially successful updates, the new list and map are only assigned after validation.

Additionally, GitLabConnectionConfig now overrides configure(StaplerRequest2, JSONObject) to avoid UI failures with no clear error messages on it. The validation exception will be caught and rethrown as FormException.

An error message for duplicate connection names already exists, so no additional message handling is required.

Hopefully, this can fix #1670. However, addConnection() still doesn't work as https://javadoc.jenkins.io/plugin/gitlab-branch-source/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.html#addServer(io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer) as described in the issue because instead of throwing an exception about it, addServer() there returns a boolean value to represent if the list is changed or not. Not sure if this is actually also wanted for addConnection() because there is no other place in the code that will examine the returned boolean value.

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

Ensure a connection is unique before adding it to the connection list.

To prevent data inconsistency caused by partially successful updates,
the new list and map are only assigned after validation.

Additionally, GitLabConnectionConfig now overrides
configure(StaplerRequest2, JSONObject) to avoid UI failures with no
clear error messages on it. The validation exception will be caught
and rethrown as FormException.

An error message for duplicate connection names already exists, so
no additional message handling is required.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
@dodo920306 dodo920306 requested a review from a team as a code owner May 17, 2025 07:44
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Test if an IllegalArgumentException will be thrown
if there are duplicate connections when
setConnections().

Signed-off-by: dodo920306 <dodo920306@gmail.com>
@github-actions github-actions bot added the tests This PR adds/removes/updates test cases label May 17, 2025
Signed-off-by: dodo920306 <dodo920306@gmail.com>
@krisstern krisstern added the enhancement For changelog: Minor enhancement. Use 'major-rfe' for changes to be highlighted label May 17, 2025
Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

This makes sense.
Thanks for the PR @dodo920306!

@dodo920306
Copy link
Contributor Author

Will you merge this?

@krisstern
Copy link
Member

I am waiting to see if anyone has any objections to this before merging this sometime over or after the weekend

@krisstern krisstern merged commit 2d15240 into jenkinsci:master May 19, 2025
17 checks passed
@dodo920306 dodo920306 deleted the fix/check-if-connection-is-unique branch May 19, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement For changelog: Minor enhancement. Use 'major-rfe' for changes to be highlighted tests This PR adds/removes/updates test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitLabConnectionConfig.does not check if Gitlab Connection is unique

2 participants