Use the owner Id only in the workspace `RuntimeIdentity` (#8859) #9274

davidfestal · 2018-03-28T10:00:21Z

What does this PR do?

This PR removes the ownerName attribute of the RuntimeIdentity class, to keep only the ownerId 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.

Signed-off-by: David Festal <dfestal@redhat.com>

codenvy-ci · 2018-03-28T10:04:02Z

Can one of the admins verify this patch?

davidfestal · 2018-03-28T10:06:59Z

ci-test

garagatyi

I think we should not add UserDao and instead should use user ID as owner value.

@skabashnyuk @sleshchenko WDYT?

sleshchenko · 2018-03-28T12:20:10Z

...re-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/spi/InternalRuntime.java

@@ -60,7 +70,12 @@ public String getActiveEnv() {

  @Override
  public String getOwner() {
-    return context.getIdentity().getOwnerName();
+    try {
+      User user = userDao.getById(context.getIdentity().getOwnerId());


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.

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.

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 😉

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.

sleshchenko · 2018-03-28T12:20:59Z

...org/eclipse/che/workspace/infrastructure/docker/environment/DockerEnvironmentNormalizer.java

      containerConfig.setContainerName(
          containerNameGenerator.generateContainerName(
              identity.getWorkspaceId(),
              containerConfig.getId(),
-              identity.getOwnerName(),
+              userName,


I'd say that using owner id here is even better. @garagatyi WDYT?

so the container naming strategy will change ? Doesn't it have any impact on UI for the end-user ?

I think no. The only one person who sees it is Che Admin using docker CLI. @garagatyi Am I right?

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.

codenvy-ci · 2018-03-28T16:05:53Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9274
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sleshchenko

... instead of the owner name. As requested by @sleshchenko in comment #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

@garagatyi

... as request by @garagatyi in this comment: #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2018-03-29T09:38:43Z

ci-test

davidfestal · 2018-03-29T09:40:18Z

@sleshchenko @garagatyi I made the 2 changes you requested to remove the use of the UserDao. I've ran the full build locally with tests and everything was OK.

Let's see what integration tests say about this change from the user name to the user Id in the runtime definition...

codenvy-ci · 2018-03-29T16:56:05Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9274
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

davidfestal · 2018-03-30T10:20:01Z

@sleshchenko @garagatyi In the second build (after UserDao removal), there are slightly more selenium tests failing, but I don't see how new failures are obviously related to this change.

Could you have a look please and approve this PR is it's really OK ?

sleshchenko · 2018-03-30T10:28:07Z

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?

garagatyi

Looks good

sleshchenko

I believe that your changes are not the reason of the failed tests.
LGTM

Use the owner Id only in the workspace RuntimeIdentity (#8859)

d411a1b

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal requested review from garagatyi and skabashnyuk as code owners March 28, 2018 10:00

davidfestal requested a review from l0rd March 28, 2018 10:00

davidfestal self-assigned this Mar 28, 2018

l0rd approved these changes Mar 28, 2018

View reviewed changes

garagatyi requested a review from sleshchenko March 28, 2018 12:16

garagatyi reviewed Mar 28, 2018

View reviewed changes

sleshchenko reviewed Mar 28, 2018

View reviewed changes

benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Mar 29, 2018

davidfestal added 2 commits March 29, 2018 10:37

use the ownerId to create the Docker container name ...

0912ebf

... instead of the owner name. As requested by @sleshchenko in comment #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

Use ownerId to return the owner of a Runtime ...

f401e9e

... as request by @garagatyi in this comment: #9274 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

garagatyi approved these changes Mar 30, 2018

View reviewed changes

sleshchenko approved these changes Mar 30, 2018

View reviewed changes

davidfestal merged commit c19ccdc into master Mar 30, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 30, 2018

benoitf added this to the 6.4.0 milestone Mar 30, 2018

davidfestal deleted the RuntimeIdentity-refactoring branch March 30, 2018 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the owner Id only in the workspace `RuntimeIdentity` (#8859) #9274

Use the owner Id only in the workspace `RuntimeIdentity` (#8859) #9274

davidfestal commented Mar 28, 2018

codenvy-ci commented Mar 28, 2018

davidfestal commented Mar 28, 2018

garagatyi left a comment

sleshchenko Mar 28, 2018

davidfestal Mar 28, 2018

sleshchenko Mar 28, 2018

garagatyi Mar 28, 2018

sleshchenko Mar 28, 2018

davidfestal Mar 28, 2018

sleshchenko Mar 28, 2018

garagatyi Mar 28, 2018

codenvy-ci commented Mar 28, 2018

davidfestal commented Mar 29, 2018

davidfestal commented Mar 29, 2018

codenvy-ci commented Mar 29, 2018

davidfestal commented Mar 30, 2018

sleshchenko commented Mar 30, 2018

garagatyi left a comment

sleshchenko left a comment

Use the owner Id only in the workspace RuntimeIdentity (#8859) #9274

Use the owner Id only in the workspace RuntimeIdentity (#8859) #9274

Conversation

davidfestal commented Mar 28, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

codenvy-ci commented Mar 28, 2018

davidfestal commented Mar 28, 2018

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Mar 28, 2018

davidfestal commented Mar 29, 2018

davidfestal commented Mar 29, 2018

codenvy-ci commented Mar 29, 2018

davidfestal commented Mar 30, 2018

sleshchenko commented Mar 30, 2018

garagatyi left a comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

Use the owner Id only in the workspace `RuntimeIdentity` (#8859) #9274

Use the owner Id only in the workspace `RuntimeIdentity` (#8859) #9274