Adding functionality to switch user terminal to dom. #15892

Katka92 · 2020-01-31T14:08:09Z

What does this PR do?

Add functionality to switch terminal to dom. This would allow us to check the text in the terminal.

What issues does this PR fix or reference?

Part of #15038

che-bot · 2020-01-31T14:37:30Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-01-31T15:02:57Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15892

che-bot · 2020-01-31T15:24:07Z

Can one of the admins verify this patch?

tests/e2e/inversify.types.ts

Ohrimenko1988 · 2020-01-31T15:57:24Z

tests/e2e/utils/PreferencesHandler.ts

+    constructor(@inject(CLASSES.RequestHandler) private readonly requestHandler: RequestHandler) {
+    }
+
+    public async setTerminalToDom() {


Method implementation looks overflowed, please try to clean up method and make it more readable.
(Simplifying logic, extracting steps to the atomic private methods ...)

yeah... I would actually do two changes here.

I would change this method to be more general (something like setTerminalType(string/enum) so we can switch back and forth if needed. This shouldn't be a big change.

I would also prefer separation to for example two methods - this one would be handling the requests (get/post) and second one would be handling the work with json (parsing&editing the json object)

I've simplified the logic, so now I don't think we need to separate it to methods. WDYT? Can you please review it again?

tests/e2e/utils/RequestHandler.ts

che-bot · 2020-02-05T13:34:40Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-02-05T14:16:28Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15892

…tion between singe/multi user approach.

che-bot · 2020-02-05T14:54:26Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

Katka92 · 2020-02-05T14:56:35Z

crw-ci-test

tests/e2e/utils/MultiUserRequestHandler.ts

che-bot · 2020-02-05T15:18:46Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

tests/e2e/utils/AbstractRequestHandler.ts

che-bot · 2020-02-05T16:27:50Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15892

che-bot · 2020-02-06T16:36:51Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2020-02-06T17:49:22Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15892

…ers.

che-bot · 2020-02-07T10:01:41Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

Ohrimenko1988 · 2020-02-07T10:06:30Z

tests/e2e/utils/requestHandlers/CheApiRequestHandler.ts

+    constructor(@inject(TYPES.HeaderHandler) private readonly headerHandler: IHeaderHandler) {
+    }
+
+    async get(url: string) : Promise<AxiosResponse> {


On my opinion the relativeUrl will be more convenient here.
Because simple url may leads to confusion,
for example if I see url I think about full url http://www.example....
but in this case, uses relative URL.

Ohrimenko1988 · 2020-02-07T10:06:56Z

tests/e2e/utils/requestHandlers/CheApiRequestHandler.ts

+        return await axios.get(this.assembleUrl(url), await this.headerHandler.getHeaders());
+    }
+
+    async post(url: string, data?: string) : Promise<AxiosResponse> {


https://github.com/eclipse/che/pull/15892/files#r376309627

Ohrimenko1988 · 2020-02-07T10:07:06Z

tests/e2e/utils/requestHandlers/CheApiRequestHandler.ts

+        }
+    }
+
+    async delete(url: string) : Promise<AxiosResponse> {


https://github.com/eclipse/che/pull/15892/files#r376309627

Ohrimenko1988 · 2020-02-07T10:07:15Z

tests/e2e/utils/requestHandlers/CheApiRequestHandler.ts

+        return await axios.delete(this.assembleUrl(url), await this.headerHandler.getHeaders());
+    }
+
+    private assembleUrl(url: string) : string {


https://github.com/eclipse/che/pull/15892/files#r376309627

Ohrimenko1988 · 2020-02-07T10:16:57Z

tests/e2e/utils/requestHandlers/CheApiRequestHandler.ts

+    async post(url: string, data?: string) : Promise<AxiosResponse> {
+        if ( data === undefined ) {
+            return await axios.post(this.assembleUrl(url), await this.headerHandler.getHeaders());
+        } else {


Redundant else

async post(url: string, data?: string): Promise<AxiosResponse> { if (data === undefined) { return await axios.post(this.assembleUrl(url), await this.headerHandler.getHeaders()); } return await axios.post(this.assembleUrl(url), data, await this.headerHandler.getHeaders()); }

Do you agree that it looks more clear than:

async post(url: string, data?: string) : Promise<AxiosResponse> { if ( data === undefined ) { return await axios.post(this.assembleUrl(url), await this.headerHandler.getHeaders()); } else { return await axios.post(this.assembleUrl(url), data, await this.headerHandler.getHeaders()); } }

I don't see much difference in this specific case. I understand that the form you are suggesting could be very beneficial in some other cases.
If it would be for my personal feeling, then here (and I emphasize here), the version with else is a bit more nicer to my eyes.

Here you can read an extended description of the rule I mentioned.
Of course, this is not a very serious resource, but if you want
I can find a more serious source.
https://www.geeksforgeeks.org/writing-clean-else-statements

I would suggest to prefer If without else where it's possible, because it shortens possible execution paths and so makes the code easier to read and understand.
This specific case Smalltalk calls "guard clauses" https://softwareengineering.stackexchange.com/a/18459

Heh... This conversation about ifs and elses is starting to be almost "decadent" :-D
Nobody is questioning, that the approach you are suggesting here and all the resources you are linking here are talking about is the right approach in general and in most of the cases, but in some specific cases writing else statement could be "neutral" (nor good, nor bad) :-D
Anyway... To close this pointless discussion... Isn't this if unecessary after all? Because axios.post has data as optional:

post<T = any>(url: string, data?: any, config?: AxiosRequestConfig): AxiosPromise<T>;

Thus we don't have to care about that at all, isn't that right?

Of course, you are right, if axios can apply null data. But I am not 100% that axios handle null data in the way we expected. Should be verified I guess.

Ok, I'll try that and if it handles null data correctly, I'll push this.

Ohrimenko1988 · 2020-02-07T10:25:52Z

tests/e2e/utils/requestHandlers/MultiUserHeaderHandler.ts

+    constructor(@inject(CLASSES.TokenHandler) private readonly tokenHandler: TokenHandler) {
+    }
+    async getHeaders() : Promise<AxiosRequestConfig> {
+        let token = await this.tokenHandler.getCheBearerToken();


According to the best practices, we should use "const" where it possible.
In this case, token will not be changed so better to use const instead let

Also, it would be great don't forget about types, so declare the "string" type here, please.

Ohrimenko1988 · 2020-02-07T10:28:52Z

tests/e2e/utils/requestHandlers/SingleUserHeaderHandler.ts

+
+@injectable()
+export class SingleUserHeaderHandler implements IHeaderHandler {
+    async getHeaders() {


getHeaders() : Promise<AxiosRequestConfig> ?

Ohrimenko1988 · 2020-02-07T10:36:55Z

tests/e2e/utils/TokenHandler.ts

+import { TestConstants } from '../TestConstants';
+
+@injectable()
+export class TokenHandler {


It also would be great to make the TokenHandler optional (ITokenHandler).
This will make it possible to fully manage queries with inversify and not think about rebuilding logic in the future.
Because for now, it looks like a bottleneck in the queries handling logic.

I'm not sure what you mean by this. Could you please elaborate a bit more?

I mean that with current changes we may easily configure all parts of the RequestHandler except token obtaining part. And I see that it may be a problem in the future. For example if we will need test "Che" with external keycloak. And this is only one example.
In this case we will not be able to reconfigure RequestHandler for successful queries
using inversify.
And I suggest to lay this opportunity.
Besides, it doesn't require a lot of changes, minutes on this stage may save hours or even days in the future.

Hmm.. Hosted che is not using keycloak.. So the logic for obtaining active token there is completely different.
@Katka92 How did you plan to deal with that on hosted che side? Did you want to create your own implementation of IHeaderHandler and there use your own logic (possible own class, as it's here) for token obtaining?
I wouldn't even be against having token obtaining part (what is now TokenHandler) implemented direclty inside MultiUserHeaderHandler.

@rhopp I was for having the code from TokenHandler directly in MultiUserHeaderHandler, but as @Ohrimenko1988 wanted it strictly separated I created this class.
When I was creating it, I was thinking about having class (e.g. HostedCheHeaderHandler) implementing IHeaderHandler which will use our own token handling logic.

@Ohrimenko1988 If I compare your solution to mine, we only differ in the fact, that you put "another layer" there. I didn't... just because I don't find this necessary.

Thinking about it now, I can think of only one reason to keep the mechanism of obtaining token separate - And that's if somebody would need to use the token without using CheApiRequestHandler... Is that why you separated this into own layer? If we have good (future) use case for this, let's go with this separation. If no, I would prefer to use "simpler" approach.

We have several different places. My variant gives us huge flexibility and the possibility to reuse all separated modules (for example "Token") in other places.
Let's agree to stop discussing about placing all to the one place, at least this approach doesn't respect SOLID principles, and I don't see any reason for intentional misconduct these principles and reducing the quality of the code.
I strongly disagree with your point that if the proposed solution resolves a specific problem we don't care about anything else. And for forwarding the best practices we need additional reasons.
Just resolve a problem don't enough for me. We should think strategically. This solution should respect the code quality, clean code principles and don't reduce tests flexibility.

ok. Let's go with your variant.
I don't particularly like the (in my opinion unecessary) overengineering, but I see I don't have any strong case against (except of simplicity).
@Katka92 Can you please act on this and provide update when you have time to do so? Or should somebody else take this responsibility?

@Ohrimenko1988 Could you please make a PR with your branch to mine? It will be easier for me to apply all the changes. I'll test it and commit changes here for the final review. Thank you

This is not overengineering, just respecting best practices.

che-bot · 2020-02-07T10:44:38Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15892
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

dmytro-ndp · 2020-02-07T14:25:46Z

@Katka92: what about storing PreferencesHandler.ts and TokenHandler.ts together with other files in the utils/requestHandlers/ directory?

Signed-off-by: Ihor Okhrimenko <[email protected]>

che-bot · 2020-02-12T08:47:36Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15892

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-02-12T09:26:46Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15892

Ohrimenko1988

Looks good for me. Thank you for collaboration.

Katka92 requested review from l0rd, nickboldt, rhopp and vparfonov as code owners January 31, 2020 14:08

che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Jan 31, 2020

Ohrimenko1988 reviewed Jan 31, 2020

View reviewed changes

tests/e2e/inversify.types.ts Outdated Show resolved Hide resolved

Ohrimenko1988 reviewed Jan 31, 2020

View reviewed changes

tests/e2e/utils/RequestHandler.ts Outdated Show resolved Hide resolved

Ohrimenko1988 reviewed Feb 4, 2020

View reviewed changes

tests/e2e/utils/RequestHandler.ts Outdated Show resolved Hide resolved

Ohrimenko1988 reviewed Feb 4, 2020

View reviewed changes

tests/e2e/utils/RequestHandler.ts Outdated Show resolved Hide resolved

Katka92 added 2 commits February 5, 2020 14:08

Adding functionality to switch user terminal to dom.

36ee63c

Simplify and parametrize setting terminal type.

1d9b28e

Katka92 force-pushed the add_terminal_switch branch from 581cc8f to 1d9b28e Compare February 5, 2020 13:13

Abstract common request handling parts, use inversify for differentia…

3069830

…tion between singe/multi user approach.

Ohrimenko1988 reviewed Feb 5, 2020

View reviewed changes

tests/e2e/utils/MultiUserRequestHandler.ts Outdated Show resolved Hide resolved

Ohrimenko1988 reviewed Feb 5, 2020

View reviewed changes

tests/e2e/utils/AbstractRequestHandler.ts Outdated Show resolved Hide resolved

Changing to one CheApiRequestHandler and to dynamicaly injecting head…

80b66cf

…ers.

Katka92 force-pushed the add_terminal_switch branch from 014a550 to 80b66cf Compare February 7, 2020 09:41

Ohrimenko1988 reviewed Feb 7, 2020

View reviewed changes

Ohrimenko1988 and others added 2 commits February 11, 2020 12:15

Add suggestions

d0c0f44

Signed-off-by: Ihor Okhrimenko <[email protected]>

Applying changes from comments.

a073ffd

Ohrimenko1988 approved these changes Feb 12, 2020

View reviewed changes

rhopp approved these changes Feb 12, 2020

View reviewed changes

Katka92 merged commit f2273b2 into eclipse-che:master Feb 12, 2020

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 12, 2020

che-bot added this to the 7.9.0 milestone Feb 12, 2020

Katka92 deleted the add_terminal_switch branch April 21, 2020 09:40

Adding functionality to switch user terminal to dom. #15892

Adding functionality to switch user terminal to dom. #15892

Conversation

Katka92 commented Jan 31, 2020

What does this PR do?

What issues does this PR fix or reference?

che-bot commented Jan 31, 2020

che-bot commented Jan 31, 2020

che-bot commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

Katka92 commented Feb 5, 2020

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

che-bot commented Feb 6, 2020

che-bot commented Feb 6, 2020

che-bot commented Feb 7, 2020

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

Ohrimenko1988 Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Feb 7, 2020 • edited Loading

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

Choose a reason for hiding this comment

rhopp Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohrimenko1988 Feb 10, 2020 • edited Loading

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

che-bot commented Feb 7, 2020

dmytro-ndp commented Feb 7, 2020

che-bot commented Feb 12, 2020

che-bot commented Feb 12, 2020

Ohrimenko1988 left a comment

Choose a reason for hiding this comment

Ohrimenko1988 Feb 7, 2020 •

edited

Loading

dmytro-ndp Feb 7, 2020 •

edited

Loading

rhopp Feb 7, 2020 •

edited

Loading

Ohrimenko1988 Feb 10, 2020 •

edited

Loading