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.

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

feat(cactus-cmd-gui-app): add common cacti gui app #2265

Closed
wants to merge 1 commit into from

Conversation

ervvkb
Copy link

@ervvkb ervvkb commented Jan 13, 2023

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

Reviewed, looks good to me.

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ervvkb I'm guessing these non-existent file paths need to be removed from the tsconfig.json file (or the non-existent files to be added but I'd prefer keeping this already large PR at it's current or smaller size rather than growing it TBH)

tsconfig.json Outdated
@@ -73,6 +76,9 @@
{
"path": "./packages/cactus-verifier-client/tsconfig.json"
},
{
"path": "./packages/cactus-plugin-persistence-ethereum/tsconfig.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

@ervvkb This file does not exist on the main branch, nor in your PR diff. Was this added by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolving because now I know that the parent PR has this in it's diff.

tsconfig.json Outdated
@@ -127,6 +133,9 @@
{
"path": "./examples/cactus-example-discounted-asset-trade/tsconfig.json"
},
{
"path": "./examples/cactus-example-fujitsu-microfinance/tsconfig.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

@ervvkb This file does not exist on the main branch, nor in your PR diff. Was this added by mistake?

Copy link
Author

Choose a reason for hiding this comment

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

I removed the path to microfinance, the second path is correct, it just wasn't merged before uploading the gui

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the path to microfinance, the second path is correct, it just wasn't merged before uploading the gui

@ervvkb Got it, thank you! Now in order to make it a little easier to review the large diff, please re-organize the commits the same way I explained it here in this other PR as well: https://github.com/hyperledger/cacti/pull/2331#pullrequestreview-1360124392

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

-Add GUI app to visualize ledger data stored in supabase by persistence plugins

Closes: hyperledger-cacti#2264
Depends on: hyperledger-cacti#2259

Signed-off-by: Eryk Baranowski eryk.baranowski@fujitsu.com
@ervvkb ervvkb requested a review from petermetz April 21, 2023 10:02
ervvkb added a commit to barnapa/cacti that referenced this pull request May 17, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
@petermetz
Copy link
Contributor

Suggested edit:

diff --git a/packages/cactus-cmd-gui-app/package.json b/packages/cactus-cmd-gui-app/package.json
index f2ef238cf..9b08c3ad1 100644
--- a/packages/cactus-cmd-gui-app/package.json
+++ b/packages/cactus-cmd-gui-app/package.json
@@ -1,27 +1,27 @@
 {
-  "name": "@hyperledger/cactus-cmd-gui-app",
-  "version": "1.1.3",
-  "description": "Cactus GUI for visualizing ledger data.",
+  "name": "@hyperledger/cacti-cmd-gui-app",
+  "version": "2.0.0-alpha.1",
+  "description": "Cacti GUI for visualizing ledger data.",
   "keywords": [
     "Hyperledger",
-    "Cactus",
+    "Cacti",
     "Integration",
     "Blockchain",
     "Distributed Ledger Technology"
   ],
-  "homepage": "https://github.com/hyperledger/cactus#readme",
+  "homepage": "https://github.com/hyperledger/cacti#readme",
   "bugs": {
-    "url": "https://github.com/hyperledger/cactus/issues"
+    "url": "https://github.com/hyperledger/cacti/issues"
   },
   "repository": {
     "type": "git",
-    "url": "git+https://github.com/hyperledger/cactus.git"
+    "url": "git+https://github.com/hyperledger/cacti.git"
   },
   "license": "Apache-2.0",
   "author": {
-    "name": "Hyperledger Cactus Contributors",
-    "email": "cactus@lists.hyperledger.org",
-    "url": "https://www.hyperledger.org/use/cactus"
+    "name": "Hyperledger Cacti Contributors",
+    "email": "cacti@lists.hyperledger.org",
+    "url": "https://www.hyperledger.org/use/cacti"
   },
   "contributors": [
     {

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ervvkb Please see my comments above and also the suggestions I made to the package.json file.


## Remarks

- Plugin requires running Supabase and persistence plugins in order to properly view ledger data.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ervvkb Would it be possible to include shell commands here on how to get that done as well? Ideally the steps that can be executed in order and then have the whole thing working.

I see that there is a "Usage" section already but it keeps it vague by saying "use this and that image" and "set this paramtere to the "correct" value" (I'm guessing it's self-evident what is the correct value if you have experience like you do, but to others that may be less helpful to just say "set it to the correct value")

It is also fine if you do it later, but then I'll ask that you create a specific issue in the tracker for it with a title something like docs(cmd-gui-app): add usage guide to README

Comment on lines +24 to +29
In the root of the project, execute the command to install and build the dependencies. It will also build this GUI front-end component:

```sh
yarn run build
```

Copy link
Contributor

Choose a reason for hiding this comment

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

@ervvkb The build command will only perform the build but won't install the dependencies AFAICT so I'd recommend having the configure script or a yarn install here as well.

The easiest way to double check if your documentation is accurate is by running the steps one by one yourself and then seeing if/where it fails. (boring but often yields great results in terms of figuring out that some commands were missing that newcomers will immediately get stuck on as they are trying to use the code)


## Getting Started

Clone the git repository on your local machine. Follow these instructions that will get you a copy of the project up and running on your local machine for development and testing purposes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ervvkb Can this not be used directly from npm via installing it to another project as a dependency? If the only way that this can be used is by copy pasting the source code of the framework then I'd call it an example rather than an actual reusable component of the framework.

If it CAN be used out of the box as an npm install into another project then please make sure to document how that can be done exactly as well (maybe you already did and I'm just not seeing it - sorry if this is the case)

petermetz pushed a commit to barnapa/cacti that referenced this pull request Jun 12, 2023
1. Add a new plugin for storing hyperledger fabric ledger data into a database
2. Add functional tests for plugin and data access layer operations.
operating with fabric-all-in-one docker composition

Tests assume any postgres database, but for final deployment postgres
or supabase is assumed.
Data fed by this plugin can later be visualized by a GUI application or
analyzed directly. ( right now the GUI will have switch to operate
either with Ethereum ledger or Fabric ledger )

Also upgrade ts-node in the rood package.json because it was causing
crashes for the scripts in the ./tools/ directory (such as the webpack
bundle name validator script)

Depends on: hyperledger-cacti#2259
Depends on: hyperledger-cacti#2265

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Barnaba Pawełczak <barnaba.pawelczak@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz pushed a commit that referenced this pull request Jun 12, 2023
1. Add a new plugin for storing hyperledger fabric ledger data into a database
2. Add functional tests for plugin and data access layer operations.
operating with fabric-all-in-one docker composition

Tests assume any postgres database, but for final deployment postgres
or supabase is assumed.
Data fed by this plugin can later be visualized by a GUI application or
analyzed directly. ( right now the GUI will have switch to operate
either with Ethereum ledger or Fabric ledger )

Also upgrade ts-node in the rood package.json because it was causing
crashes for the scripts in the ./tools/ directory (such as the webpack
bundle name validator script)

Depends on: #2259
Depends on: #2265

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Barnaba Pawełczak <barnaba.pawelczak@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
barnapa added a commit to barnapa/cacti that referenced this pull request Jun 15, 2023
1. Add a new plugin for storing hyperledger fabric ledger data into a database
2. Add functional tests for plugin and data access layer operations.
operating with fabric-all-in-one docker composition

Tests assume any postgres database, but for final deployment postgres
or supabase is assumed.
Data fed by this plugin can later be visualized by a GUI application or
analyzed directly. ( right now the GUI will have switch to operate
either with Ethereum ledger or Fabric ledger )

Also upgrade ts-node in the rood package.json because it was causing
crashes for the scripts in the ./tools/ directory (such as the webpack
bundle name validator script)

Depends on: hyperledger-cacti#2259
Depends on: hyperledger-cacti#2265

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Barnaba Pawełczak <barnaba.pawelczak@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
barnapa added a commit to barnapa/cacti that referenced this pull request Jun 15, 2023
1. Add a new plugin for storing hyperledger fabric ledger data into a database
2. Add functional tests for plugin and data access layer operations.
operating with fabric-all-in-one docker composition

Tests assume any postgres database, but for final deployment postgres
or supabase is assumed.
Data fed by this plugin can later be visualized by a GUI application or
analyzed directly. ( right now the GUI will have switch to operate
either with Ethereum ledger or Fabric ledger )

Also upgrade ts-node in the rood package.json because it was causing
crashes for the scripts in the ./tools/ directory (such as the webpack
bundle name validator script)

Depends on: hyperledger-cacti#2259
Depends on: hyperledger-cacti#2265

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Barnaba Pawełczak <barnaba.pawelczak@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
barnapa pushed a commit to barnapa/cacti that referenced this pull request Jun 22, 2023
…rledger ledger data stored in supabase by persistence plugins

Depends on: hyperledger-cacti#2265
Depends on: hyperledger-cacti#2331

Signed-off-by: Barnaba Pawelczak barnaba.pawelczak@fujitsu.com
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ervvkb Are you still working on this?

@ervvkb
Copy link
Author

ervvkb commented Sep 3, 2023

@ervvkb Are you still working on this?

I was transfered to another project couple months ago, so nope, thought someone took care of it

@petermetz
Copy link
Contributor

@ervvkb Are you still working on this?

I was transfered to another project couple months ago, so nope, thought someone took care of it

@ervvkb Please let us know next time so that we don't waste hours reviewing a pull request that was abandoned?

@petermetz petermetz closed this Sep 8, 2023
@outSH
Copy link
Contributor

outSH commented Sep 11, 2023

@petermetz This was merged as part of https://github.com/hyperledger/cacti/pull/2363, I did not catch that this PR is still open - sorry for confusion

@petermetz
Copy link
Contributor

@petermetz This was merged as part of #2363, I did not catch that this PR is still open - sorry for confusion

@outSH No worries! I'm sorry for the slow response! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(cactus-cmd-gui-app): add common cacti gui app
5 participants