Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add piggyback commands to ui #1791

Merged
merged 8 commits into from
Nov 14, 2023
Merged

Add piggyback commands to ui #1791

merged 8 commits into from
Nov 14, 2023

Conversation

vvasilevbosch
Copy link
Contributor

No description provided.

@thjaeckle
Copy link
Member

Very cool to see this enhancement, thanks @vvasilevbosch

@thfries would be great if you could have a look at the PR. It adds something we used to have in the commercial devops UI, a templating mechanism to select and configure Ditto devops commands and execute them with Admin permissions.

I can also review, however I am not so familiar with the UI yet.

@thjaeckle thjaeckle added the UI Issues related to the Ditto explorer UI label Nov 1, 2023
@thfries
Copy link
Contributor

thfries commented Nov 2, 2023

Hi, @vvasilevbosch, @thjaeckle,

Sure, love to do that. Glad to see these extensions. Thanks, @vvasilevbosch

thfries
thfries suggested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@thfries thfries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vvasilevbosch, very nice! Looks like quite some effort.
I made some suggestions.

One major decision is if this should be included into the Operations tab or if it should stay a separate tab (@thjaeckle , wdyt?)

I m not used to operating ditto, so I can not judge usability here very well.

One thought from my side: there are 2 "service" selections, one on the main page and one in the modal. I was thinking if we could keep just one service selection on the main page and include the template selection below the service selection. Thus we may not need a separate modal. Just one dropdown for the template.?

ui/index.html Outdated Show resolved Hide resolved
ui/main.scss Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.ts Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.ts Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.html Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.html Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.html Outdated Show resolved Hide resolved
ui/modules/piggyback/piggyback.html Outdated Show resolved Hide resolved
@thjaeckle
Copy link
Member

One major decision is if this should be included into the Operations tab or if it should stay a separate tab (@thjaeckle , wdyt?)

I think it should go into the "Operations" tab - I would not want to "clutter" the top-menu with more items ..
And it is very related to the "logging" which is already in there.
We probably need then a second navigation for "Operations", right?

@thfries
Copy link
Contributor

thfries commented Nov 3, 2023

One major decision is if this should be included into the Operations tab or if it should stay a separate tab (@thjaeckle , wdyt?)

I think it should go into the "Operations" tab - I would not want to "clutter" the top-menu with more items .. And it is very related to the "logging" which is already in there. We probably need then a second navigation for "Operations", right?

I haven't thought about a second navigation yet. I would do it like on the connections tab, with one almost full screen for editing the connection and below the area to monitor the connection. You need to scroll down the page

@vvasilevbosch
Copy link
Contributor Author

@thfries @thjaeckle, I have applied the suggested changes. I left out moving the piggyback commands to the operations tab for now, since it's still under discussion.

@thjaeckle
Copy link
Member

I haven't thought about a second navigation yet. I would do it like on the connections tab, with one almost full screen for editing the connection and below the area to monitor the connection. You need to scroll down the page

That is also better than having it at top level, so I would also suggest doing it that way.

@thjaeckle thjaeckle added this to the 3.5.0 milestone Nov 6, 2023
@vvasilevbosch
Add piggyback commands to ui
ec3a108 
Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch
Move try catch block to correct place, make sure onRequestDone is cal…
4237843 
…led in case of callDittoREST method error thrown

Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch
Move stringifyPretty to utils, replace all JSON.stringify usage with …
f71113b 
…util function.

Apply suggested changes

Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch
Moved piggyback to operations tab
51d7a0b 
Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch vvasilevbosch force-pushed the feature/ui-add-piggyback-commands branch from 635ea54 to 51d7a0b Compare November 7, 2023 13:25
@vvasilevbosch
add newlines at end of files
42c8cbc 
Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@thfries
Copy link
Contributor

thfries commented Nov 7, 2023

Very nice @vvasilevbosch, looks good.

I propose to

  • also move the piggyback files to the operations folder and to rename the existing operations files to servicesLogging
  • reduce the height of the piggyback area so that you can see the headline of the logging part (equal to on the connections page)
    I added a patch file for these proposals. Please take a look.
    0001-UI-piggyback-commands-review.patch

@vvasilevbosch
move piggyback files to operations/, rename operations to serviceLogging
8e6dfd7 
Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch
Copy link
Contributor Author

@thfries @thjaeckle I have commited the proposals, but I found a small bug in the functionality that I need to fix, also I must remove some piggyback commands, that are not available in ditto, e.g. in search service - index namespace

@vvasilevbosch
Fix path for piggyback request, remove commands not part of ditto
fd70011 
Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch vvasilevbosch requested a review from thfries November 8, 2023 11:11
thfries
thfries approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@thfries thfries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks @vvasilevbosch

ui/modules/utils.ts Outdated Show resolved Hide resolved
@vvasilevbosch
Remove util method for set editor value, replace calls with setEditor…
0d1dc7b 
…Value(value, -1)

Signed-off-by: Vasil Vasilev <vasil.vasilev@bosch.com>
@vvasilevbosch
Copy link
Contributor Author

@thjaeckle I think this is ready for merge, is there something from my side that should be done?

@thjaeckle thjaeckle merged commit 22e9a35 into eclipse-ditto:master Nov 14, 2023
2 checks passed
@vvasilevbosch vvasilevbosch deleted the feature/ui-add-piggyback-commands branch November 16, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thfries thfries thfries approved these changes

Assignees

@vvasilevbosch vvasilevbosch

Labels
UI Issues related to the Ditto explorer UI
Projects
Ditto Planning
Status: Done
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@vvasilevbosch @thjaeckle @thfries