-
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
Use the owner Id only in the workspace RuntimeIdentity
(#8859)
#9274
Conversation
Signed-off-by: David Festal <dfestal@redhat.com>
Can one of the admins verify this patch? |
ci-test |
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.
I think we should not add UserDao and instead should use user ID as owner value.
@skabashnyuk @sleshchenko WDYT?
@@ -60,7 +70,12 @@ public String getActiveEnv() { | |||
|
|||
@Override | |||
public String getOwner() { | |||
return context.getIdentity().getOwnerName(); | |||
try { | |||
User user = userDao.getById(context.getIdentity().getOwnerId()); |
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.
I don't think it is good to return owner name or owner id here. Let's document that owner id will be returned in Runtime
class and just return it.
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.
I don't think it is good to return owner name or owner id here
Let's be clear, returning the ownerId
here is just a way to gracefully manage an abnormal and exceptional case. Maybe we could just throw an exception instead.
Let's document that owner id will be returned in Runtime class and just return it.
That's quite an impacting change, no ?
Change the meaning of value from a plain user name to an ID (opaque to the user), in a location that is used in many places (even in the RuntimeDto
). This will surely produce some behavior changes, also in the UI and for the end-user.
That seems strange to me.
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.
Maybe we could just throw an exception instead.
You can try to do it but I think it might be difficult with current classes hierarchy
Change the meaning of value from a plain user name to an ID (opaque to the user), in a location that is used in many places (even in the RuntimeDto).
Makes sense. But I think using username there is a mistake that was done because of docs lack.
@gazarenkov should be right person who knows what is supposed to be there.
If user id then I think we should try to change it and run QA tests. I guess clients don't use it and everything will be OK 😉
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.
I think we agreed in your previous PR that we should do this change. I don't think that clients use it somehow now. But running tests, indeed, should verify this.
containerConfig.setContainerName( | ||
containerNameGenerator.generateContainerName( | ||
identity.getWorkspaceId(), | ||
containerConfig.getId(), | ||
identity.getOwnerName(), | ||
userName, |
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.
I'd say that using owner id here is even better. @garagatyi WDYT?
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.
so the container naming strategy will change ? Doesn't it have any impact on UI for the end-user ?
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.
I think no. The only one person who sees it is Che Admin using docker CLI. @garagatyi Am I right?
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.
I agree that container name is the only place. And it doesn't make much sense to keep username there if it costs us something.
ci-test build report: |
... instead of the owner name. As requested by @sleshchenko in comment #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>
... as request by @garagatyi in this comment: #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>
ci-test |
@sleshchenko @garagatyi I made the 2 changes you requested to remove the use of the Let's see what integration tests say about this change from the user name to the user Id in the runtime definition... |
ci-test build report: |
@sleshchenko @garagatyi In the second build (after Could you have a look please and approve this PR is it's really OK ? |
I think there are some unstable tests. @eclipse/eclipse-che-qa Could you please check them, maybe it is needed to run selenium tests one more time? |
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.
Looks good
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.
I believe that your changes are not the reason of the failed tests.
LGTM
What does this PR do?
This PR removes the
ownerName
attribute of theRuntimeIdentity
class, to keep only theownerId
according to what was requested in the following discussion.This is done while taking care of still using the owner name when it was previously used (in the container name, etc...)
All the tests pass, and the Che server + workspace start works correctly on Openshift DevCluster.
What issues does this PR fix or reference?
This fixes issue #8859
Release Notes
Nothing to add in the release notes, since it's internal refactoring
Docs PR
Nothing to add in the docs, since it's internal refactoring.