Skip to content
This repository has been archived by the owner on Aug 3, 2019. It is now read-only.

[JENKINS-57880] Feature add unique name #5

Merged
merged 4 commits into from
Jun 6, 2019
Merged

[JENKINS-57880] Feature add unique name #5

merged 4 commits into from
Jun 6, 2019

Conversation

baymac
Copy link
Owner

@baymac baymac commented Jun 6, 2019

  • The GitLabServers filter is based on a unique name given to the server at Global Configuration. Gitea Plugin does it based on serverUrl but user may sometimes want to add multiple server configuration for the same serverUrl. So I have added a function which generates a random name for the GitLabServer. User may change the name, but if user want to change it to a name that already exists there isn't a way to prevent it. This will cause problems when calling removeServer, updateServer methods. You may take a look at the issue to learn more why checking is not possible. For now this is a viable solution, as user will rarely want to change the gitlab server name.

  • Also added a removeServer method which can be used to remove server from configuration based on GitLabServer name.

*/
public synchronized boolean removeServer(@CheckForNull String name) {
boolean modified = false;
List<GitLabServer> endpoints = new ArrayList<>(getServers());
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you thought about using a HashTable to store the list of servers? That would make maintaining it simpler.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, thought of it. But having an extra data structure for a few server connections (unless I see a use case where one user wants large number of server configurations) is an tradeoff where space-complexity might be more than time-complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is called so rarely, and the space is so small, that I don't think the trade-off matters. I'd go for simplicity of code

Copy link
Collaborator

Choose a reason for hiding this comment

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

In production code, most of the time the most expensive thing is time programmers spend maintaining it :-)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, I will implement it then. 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have limited knowledge of JCasC. If you have some time, can you implement a small part of what you want with JCasC support in this plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

I want a simple test that confirms you can configure gitlabservers which is a global configuration and export.

You can configure any plugin in JCasC from a yaml file.

You should be intimate with the global configuration of the gitlab plugin, here you can see a demo for how to configure it in JCasC: https://github.com/jenkinsci/configuration-as-code-plugin/tree/master/demos/gitlab

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll be happy to send a PR.

Though I don't see any CI on the project 😢
I'll add that too if you don't mind. We have a decent Travis file on JCasC: https://github.com/jenkinsci/configuration-as-code-plugin/blob/master/.travis.yml

Copy link
Owner Author

Choose a reason for hiding this comment

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

That would be great. Thanks. :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the info about JCasC @Casz - definitely want to be compatible.

@jetersen
Copy link
Contributor

jetersen commented Jun 6, 2019

If I may be so bold and suggest that you add a JCasC config import/export test perhaps a separate PR.😓

@baymac
Copy link
Owner Author

baymac commented Jun 6, 2019

If I may be so bold and suggest that you add a JCasC config import/export test perhaps a separate PR.

Okay @Casz, I will take a look at it. Could you join tomorrow's meeting to discuss about it?

@baymac
Copy link
Owner Author

baymac commented Jun 6, 2019

Added the issue for JCasC test support. Here - https://issues.jenkins-ci.org/browse/JENKINS-57886

@baymac
Copy link
Owner Author

baymac commented Jun 6, 2019

I am saving all the requested changes for another pr. Merging this now as we all agree with the name based filters for GitLab servers.

Summary of future PRs:

  1. Add hashMap or hashTable for GitLab servers - https://issues.jenkins-ci.org/browse/JENKINS-57889
  2. Add JCasC test support - https://issues.jenkins-ci.org/browse/JENKINS-57886
  3. Remove synchronization from GitLabServers method - https://issues.jenkins-ci.org/browse/JENKINS-57892

@baymac baymac merged commit 3cb90c3 into develop Jun 6, 2019
@baymac baymac deleted the unique-name branch June 6, 2019 21:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants