-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Migrated [JenkinsPlugin] to new service architecture #3317
Conversation
Wow you turned that around fast! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM. The only question I have is around regularUpdate
since the file containing it currently resides within the legacy
directory hierarchy. If that helper will continue to be leveraged in the new service architecture, should that file (eventually) be moved to a different directory?
I think regular-update needs a total rewrite using async/await. Caching code is notoriously difficult to test and so I'd rather base the cache part on If this is the last legacy service to use it, the time to rewrite may be upon us! |
Yes, I believe this is the last legacy service that is using |
Do we want to do that here as part of refactoring this service? Or should we go ahead and merge this and then in a follow on tackle rewriting regular-update (and updating this service accordingly)? |
Think that can definitely be a follow-on. |
See #2863.