-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added MachineTokenProvider interface with different implementations #6700
Conversation
Build # 4122 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4122/ to view the results. |
f9b7581
to
7ba51ef
Compare
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4126/ |
this.runtimeIdentity = runtimeIdentity; | ||
this.machineName = machineName; | ||
this.servers = servers; | ||
this.serverCheckerFactory = serverCheckerFactory; | ||
this.timer = new Timer("ServerReadinessChecker", true); |
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.
Can you align string constant with the new new name
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.
In overall looks good. I would try to avoid interceptor, but since it is not your code we can discuss it separately.
Please, make sure that it is tested on different infrastructures and deployments.
RuntimeIdentity runtimeIdentity, | ||
String machineName, | ||
Map<String, ? extends Server> servers, | ||
ServerCheckerFactory serverCheckerFactory) { | ||
@Assisted MachineTokenProvider machineTokenProvider) { |
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.
Am I wrong that assisted annotation should be on other arguments, but not on this one?
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.
You're correct. I've already fixed it locally.
It is implemented in different ways for single and multiuser packaging
Build # 4139 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4139/ to view the results. |
What does this PR do?
Adds MachineTokenProvider interface. There are different implementations for single and multiuser packagings. With these changes, there is no necessity to override anything for implementations of infrastructures in
che multiuser
assembly.It also contains minor cleaning up of jpa tests and OpenShift provisioning.
What issues does this PR fix or reference?
#6587