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

[JENKINS-57889] Add HashMap for GitLab filter methods #15

Closed
wants to merge 9 commits into from

Conversation

baymac
Copy link
Owner

@baymac baymac commented Jun 11, 2019

  1. Added a hashmap that stores (name, server) key, value pair. Kept it transient so that we do not have to serialize it. But the problem is you cannot populate the HashMap in the constructore as some JCasC rules error. Maybe @Casz can englighten what is going wrong.

  2. Some minor documentation changes

  3. Removed gitlab server warnings because a URI check scheme with gitlab might not work. See A public endpoint to validate the GitLab server url gitlab4j/gitlab4j-api#384. I haven't been able to figure a way to validate a gitlab server url without authentication yet.

@baymac
Copy link
Owner Author

baymac commented Jun 11, 2019

Removed serverMap from transient. That might solve the problem but there will be overhead of serialising the map as well. I hope that will be fine if it saves user's time.

@jetersen
Copy link
Contributor

Could you share the exception?

But as I recall, Map is not supported: jenkinsci/configuration-as-code-plugin#900

@baymac baymac changed the title [JENKINS-57889] Added HashMap for GitLab filter methods [JENKINS-57889] Add HashMap for GitLab filter methods Jun 12, 2019
@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

You can see the error in the test results - https://gist.github.com/baymac/c26381fa04797c377fd94f4986a740f4

@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

You can also reproduce yourself by commenting out the refreshServerMap method in the GitLabServer constructor.

@jetersen
Copy link
Contributor

@baymac CI says otherwise, did you forget to do an mvn clean?

@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

The test passes in CI because the code that has been pushed, the constructor doesn't have the refreshServerMap method. Only when you include this method in the GitLabServers constructor you see the above error.

Copy link
Contributor

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Regarding using a hashmap, I feel like your pre-optimizing for a use case that is never going to be relevant. Most users will have two servers, gitlab.com, and their private instance.
With 10 or 100 servers in the list, even though that will most likely be an unrealistic scenario, an array lookup is still going to be pretty fast.

@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

In a case where there is big team and they all want to setup a server for each of their user to manage webhooks on the project they created, there will be multiple instances of server entries. I think if it is not causing compatibility issues we can benefit from hashmaps.

@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

  • Added a method removeDuplicateServers which gets called inside configure. I have also added logging so that you can see that server names with duplicate names are removed.

  • Modified refreshServerMap method to use Java 8 streams.

The problem I am still having is that servers list saves only the distinct objects (see logs) but the same is not reflected in the UI. The UI still has servers with the duplicate names. Only after Jenkins restart the UI changes occur. Is there a way to refresh the UI to reload the elements at runtime?

@baymac baymac added the Do Not Merge Wait for the removal of this label to merge label Jun 12, 2019
@jetersen
Copy link
Contributor

jetersen commented Jun 12, 2019

On a big team they would use an individual Jenkins instance as well 😓
Also usually most people have a CI user that they can just give access to their projects.

@baymac
Copy link
Owner Author

baymac commented Jun 12, 2019

Okay, I get it now. I am closing this PR. Can you review #16?

@baymac baymac closed this Jun 12, 2019
@baymac baymac deleted the hash-servers branch June 12, 2019 18:05
@baymac baymac restored the hash-servers branch June 12, 2019 18:05
@baymac baymac deleted the hash-servers branch July 16, 2019 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Do Not Merge Wait for the removal of this label to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants