Changes how the investigation tab displays the git log output #3993

b10n1k · 2021-06-30T11:39:07Z

This PR unifies the output of the git logs and git log stats into one
https://progress.opensuse.org/issues/91878

The purpose is to have a better view of what files have been changed for each
commit.

Example:

test_log

6a44799cb test
 lib/bootbasetest.pm     | 2 +-
 lib/opensusebasetest.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Signed-off-by: ybonatakis ybonatakis@suse.com

Martchus · 2021-06-30T11:43:44Z

s/Changes/Change/
The commit contains some unrelated changes.

b10n1k · 2021-06-30T11:44:18Z

b10n1k · 2021-06-30T11:48:16Z

The commit contains some unrelated changes.

i was wondering if i have to run the tidy before i submit. i can revert those. so no tidy?

okurz

So personally as explained in the ticket already I am not convinced that this approach is beneficial in general. Also I would like to be careful changing an existing behaviour unconditionally. I am open to be convinced by multiple power users though if they like your proposed format better :) As an alternative add the new view based on a configurable feature switch. As suggested in the ticket we should go with https://progress.opensuse.org/issues/92731 first before considering changing the format. @b10n1k maybe this is something that you would like to try out first?

b10n1k · 2021-06-30T12:09:39Z

s/Changes/Change/

Done

The commit contains some unrelated changes.

Reverted

b10n1k · 2021-06-30T12:14:14Z

So personally as explained in the ticket already I am not convinced that this approach is beneficial in general. Also I would like to be careful changing an existing behaviour unconditionally. I am open to be convinced by multiple power users though if they like your proposed format better :) As an alternative add the new view based on a configurable feature switch. As suggested in the ticket we should go with https://progress.opensuse.org/issues/92731 first before considering changing the format. @b10n1k maybe this is something that you would like to try out first?

https://progress.opensuse.org/issues/92731 is very cool to have but it might take more time for me. So i found some time to implement this as i had it in mind, and go as you had said "to discuss over code rather just english text"

b10n1k · 2021-07-23T11:17:02Z

After the latest commit the results should be shown as this example http://aquarius.suse.cz:7900/tests/56#investigation

there are some things still to fixed to become a proper code (and testing apparently). but i would like to get some early feedbacks

Martchus

The style check is failing. You can run tools/js-tidy locally to fix this.

assets/javascripts/test_result.js

b10n1k · 2021-07-23T12:43:49Z

@Martchus i read your comments and i will address. i just have another question.
I wonder if the hardcoded repo url can be fetched from the controller or something.

Martchus · 2021-07-23T13:11:56Z

I wonder if the hardcoded repo url can be fetched from the controller or something.

You mean for the sake of making it configurable (via the web UI service's config file)? That would make sense, we do that already in several pages, e.g. templates/webapi/admin/audit_log/productlog.html.ep and templates/webapi/step/edit.html.ep (the block started with % content_for 'ready_function' => begin).

okurz

I still prefer the previous concise list but maybe you can make use of collapsing to keep the concise list and unfold the entries on click? We already use this "more …" label. Maybe this can be a starting point for you

b10n1k · 2021-07-24T10:55:29Z

I still prefer the previous concise list but maybe you can make use of collapsing to keep the concise list and unfold the entries on click? We already use this "more …" label. Maybe this can be a starting point for you

thats a good idea. thanks

b10n1k · 2021-07-25T17:32:27Z

okurz · 2021-07-26T07:17:22Z

can you replace the leading "+" with a "-" if unfolded?

b10n1k · 2021-07-26T08:39:29Z

can you replace the leading "+" with a "-" if unfolded?

Actually i have. But i have some js strange behavior and i havent found a way through it yet. But the switch of the sign is there

assets/javascripts/test_result.js

lib/OpenQA/Utils.pm

okurz · 2021-10-14T12:58:50Z

The simple answer is no. And that was my plan from the beginning. The reason was because a change in any file is not connected with a failure necessarily (the change can be unrelated). I think it is more easy to go through the commits (good title and then the description and finally the related modified files) to find the main cause.

Well, I still see it as easier to do a single click to find the complete diff. But my suggestion was that you would simply offer both ways in the separate parts of the investigation tab. So if you bring back the old behaviour for the complete diff and just add the new functionality within the commit list then we can find an agreement :)

Martchus

The last commit message is a bit "low-level" and lacks proper casing. Since you've updated your changes it would be nice to know what you've actually changed (also regarding @okurz 's review comments).

templates/webapi/test/result.html.ep

b10n1k · 2021-11-05T11:27:25Z

The simple answer is no. And that was my plan from the beginning. The reason was because a change in any file is not connected with a failure necessarily (the change can be unrelated). I think it is more easy to go through the commits (good title and then the description and finally the related modified files) to find the main cause.

Well, I still see it as easier to do a single click to find the complete diff. But my suggestion was that you would simply offer both ways in the separate parts of the investigation tab. So if you bring back the old behaviour for the complete diff and just add the new functionality within the commit list then we can find an agreement :)

i did some changes but i am not completed sure if this what you meant. ready for review

mergify · 2021-11-08T10:42:06Z

This pull request is now in conflicts. Could you fix it? 🙏

okurz

Overall code looks fine so far. Do you have a test instance available where we could try this out?

01c5da6 deletes some parts which you bring back with fa11567 . I suggest you combine these changes into one commit but don't squash all commits.

b10n1k · 2021-11-09T15:42:38Z

@okurz i have the local dev one. Let me know when you want to take a look

okurz

I checked it on @b10n1k's instance and attached the screenshot to the PR description. Looks good. Great work! Approved :)

Martchus · 2021-11-11T10:23:25Z

lib/OpenQA/WebAPI/Controller/Test.pm

+            testgiturl => gitrepodir(distri => $job->DISTRI),
+            needlegiturl => gitrepodir(distri => $job->DISTRI, needles => 1),


Likely my last comment got lost. I still think these URLs should be rendered on the tab's page (and not the overall page).

This PR unifies the output of the git logs and git log stats into one https://progress.opensuse.org/issues/91878 The purpose is to have a better view of what files have been changed for each commit. Example: ``` test_log 6a44799cb test lib/bootbasetest.pm | 2 +- lib/opensusebasetest.pm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) ``` Signed-off-by: ybonatakis <ybonatakis@suse.com>

Before test_log be rendered i convert each hash to a link element to the actual commit in github. If more than five commits exists it concatenates the rest and they append the preElementMore directly are shown as the textLinesRest do with the `Show More` link. Signed-off-by: ybonatakis <ybonatakis@suse.com>

As part of the changes for the investigation tab with links to the commits, it was neccessity to find a way to make the repo url self-agnostic, for the test repo as well as the needles. This is what the purpose of `gitrepodir()` in *Utils.pm*. Running that, it will return something like "https://github.com/b10n1k/os-autoinst-distri-opensuse/commit/" constructed by the of the local **.git/config** and the info under the `origin` branch. `origin` might not be the one that is set so this may need to be configurable, but i leave it for later. The reason of this approach is because: - avoid static variables. In this case i could set the variables in the js and ask to use each one correspondingly. - avoid openqa setup for every instance. Similar with the previous but the variables would have to adjust for every instance and finally passed to somehow to the js. Also i stash the variables (*needlesgiturl* and *testgiturl*) inside `_show`. i think this will give access to the variables also for other utulities. However i come up with it because i strungled to stash the variables from `investigation()`. Signed-off-by: ybonatakis <ybonatakis@suse.com>

applied in other files via `make update-deps` Signed-off-by: ybonatakis <ybonatakis@suse.com>

Here also i solved a problem which i missed before with the test_log when this is just one. the problem was in the regex logic which it was always matching only when another git_log was shown. The regex used to make enumeration of the logs returned by `git log` Signed-off-by: ybonatakis <ybonatakis@suse.com>

Rather stash the variables which contain the git url on each rendering via `_show`, we move the invocation in the `investigation`. In such way, the variables are in the `investigation_json` and can be used to set the required attributes during the rendering of that particular tab. Signed-off-by: ybonatakis <ybonatakis@suse.com>

okurz requested changes Jun 30, 2021

View reviewed changes

b10n1k force-pushed the investigation_log branch 2 times, most recently from 2f2b83a to 4216ec4 Compare June 30, 2021 12:08

okurz force-pushed the investigation_log branch from 4216ec4 to 083c655 Compare July 12, 2021 12:56

b10n1k force-pushed the investigation_log branch from 083c655 to bd291bd Compare July 23, 2021 11:05

This comment has been minimized.

Sign in to view

b10n1k force-pushed the investigation_log branch from bd291bd to 4262d5c Compare July 23, 2021 11:12

b10n1k changed the title ~~Changes how the investigation tab displays the git log output~~ [WIP] Changes how the investigation tab displays the git log output Jul 23, 2021

b10n1k requested a review from okurz July 23, 2021 11:18

Martchus reviewed Jul 23, 2021

View reviewed changes

assets/javascripts/test_result.js Outdated Show resolved Hide resolved

assets/javascripts/test_result.js Outdated Show resolved Hide resolved

assets/javascripts/test_result.js Outdated Show resolved Hide resolved

b10n1k force-pushed the investigation_log branch from 4262d5c to 75566da Compare July 23, 2021 14:10

okurz requested changes Jul 23, 2021

View reviewed changes

b10n1k force-pushed the investigation_log branch 3 times, most recently from 2c74582 to 70db21f Compare July 25, 2021 17:30

b10n1k force-pushed the investigation_log branch from 70db21f to 3e9886d Compare August 4, 2021 13:58

Martchus reviewed Aug 4, 2021

View reviewed changes

assets/javascripts/test_result.js Outdated Show resolved Hide resolved

assets/javascripts/test_result.js Outdated Show resolved Hide resolved

b10n1k force-pushed the investigation_log branch from 3e9886d to 31da233 Compare August 5, 2021 13:45

b10n1k force-pushed the investigation_log branch 2 times, most recently from dfd2ee9 to 8c788fd Compare October 14, 2021 07:40

Martchus requested changes Oct 14, 2021

View reviewed changes

lib/OpenQA/Utils.pm Outdated Show resolved Hide resolved

b10n1k force-pushed the investigation_log branch from 8c788fd to d8b4b60 Compare October 14, 2021 12:57

b10n1k force-pushed the investigation_log branch from e8c380e to 736abd9 Compare November 3, 2021 09:16

Martchus requested changes Nov 3, 2021

View reviewed changes

templates/webapi/test/result.html.ep Outdated Show resolved Hide resolved

b10n1k force-pushed the investigation_log branch 3 times, most recently from 5c2fd29 to ac9671d Compare November 3, 2021 19:37

b10n1k requested a review from okurz November 4, 2021 07:41

b10n1k force-pushed the investigation_log branch from ac9671d to fa11567 Compare November 9, 2021 13:25

okurz requested changes Nov 9, 2021

View reviewed changes

okurz approved these changes Nov 9, 2021

View reviewed changes

b10n1k force-pushed the investigation_log branch from fa11567 to 6c17cbf Compare November 9, 2021 17:24

okurz approved these changes Nov 9, 2021

View reviewed changes

b10n1k requested a review from Martchus November 10, 2021 18:59

Martchus requested changes Nov 11, 2021

View reviewed changes

b10n1k added 6 commits November 15, 2021 11:33

Update dependencies.yaml with Config::Tiny

e4cb0c9

applied in other files via `make update-deps` Signed-off-by: ybonatakis <ybonatakis@suse.com>

b10n1k force-pushed the investigation_log branch from 7dc4a53 to c36c13a Compare November 15, 2021 10:34

Martchus approved these changes Nov 15, 2021

View reviewed changes

mergify bot merged commit 83ee395 into os-autoinst:master Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes how the investigation tab displays the git log output #3993

Changes how the investigation tab displays the git log output #3993

b10n1k commented Jun 30, 2021 •

edited by okurz

Loading

Martchus commented Jun 30, 2021

b10n1k commented Jun 30, 2021

b10n1k commented Jun 30, 2021 •

edited

Loading

okurz left a comment

b10n1k commented Jun 30, 2021

b10n1k commented Jun 30, 2021

This comment has been minimized.

b10n1k commented Jul 23, 2021

Martchus left a comment

b10n1k commented Jul 23, 2021

Martchus commented Jul 23, 2021 •

edited

Loading

okurz left a comment

b10n1k commented Jul 24, 2021

b10n1k commented Jul 25, 2021 •

edited

Loading

okurz commented Jul 26, 2021

b10n1k commented Jul 26, 2021

okurz commented Oct 14, 2021

Martchus left a comment

b10n1k commented Nov 5, 2021 •

edited

Loading

mergify bot commented Nov 8, 2021

okurz left a comment

b10n1k commented Nov 9, 2021

okurz left a comment

Martchus Nov 11, 2021

		testgiturl => gitrepodir(distri => $job->DISTRI),
		needlegiturl => gitrepodir(distri => $job->DISTRI, needles => 1),

Changes how the investigation tab displays the git log output #3993

Changes how the investigation tab displays the git log output #3993

Conversation

b10n1k commented Jun 30, 2021 • edited by okurz Loading

Martchus commented Jun 30, 2021

b10n1k commented Jun 30, 2021

b10n1k commented Jun 30, 2021 • edited Loading

okurz left a comment

Choose a reason for hiding this comment

b10n1k commented Jun 30, 2021

b10n1k commented Jun 30, 2021

This comment has been minimized.

b10n1k commented Jul 23, 2021

Martchus left a comment

Choose a reason for hiding this comment

b10n1k commented Jul 23, 2021

Martchus commented Jul 23, 2021 • edited Loading

okurz left a comment

Choose a reason for hiding this comment

b10n1k commented Jul 24, 2021

b10n1k commented Jul 25, 2021 • edited Loading

okurz commented Jul 26, 2021

b10n1k commented Jul 26, 2021

okurz commented Oct 14, 2021

Martchus left a comment

Choose a reason for hiding this comment

b10n1k commented Nov 5, 2021 • edited Loading

mergify bot commented Nov 8, 2021

okurz left a comment

Choose a reason for hiding this comment

b10n1k commented Nov 9, 2021

okurz left a comment

Choose a reason for hiding this comment

Martchus Nov 11, 2021

Choose a reason for hiding this comment

b10n1k commented Jun 30, 2021 •

edited by okurz

Loading

b10n1k commented Jun 30, 2021 •

edited

Loading

Martchus commented Jul 23, 2021 •

edited

Loading

b10n1k commented Jul 25, 2021 •

edited

Loading

b10n1k commented Nov 5, 2021 •

edited

Loading