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(fabric-driver): added weaver fabric driver as cacti package #2963

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

sandeepnRES
Copy link
Member

@sandeepnRES sandeepnRES commented Dec 22, 2023

  • This commit is a step towards of our roadmap of creating an integrated API, where fabric driver from weaver will be merged with ledger connector from cactus.
  • Currently this is only creating a package for the fabric driver with a symbolic link, trying to experiment how to move the weaver packages as cacti packages.
  • Following this we want to convert all the weaver modules to cacti packages.
  • And then deeper integration will be the future steps.

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

@VRamakrishna
Copy link
Contributor

@sandeepnRES Can you edit the opening comment and add some description (in bullet points) about what this feature is and how it is part of the Cacti integration roadmap?

@VRamakrishna
Copy link
Contributor

Added package LGTM. Please add some descriptions as per my comments before merging though.

packages/cacti-weaver-driver-fabric/package.json Outdated Show resolved Hide resolved
packages/cacti-weaver-driver-fabric/package.json Outdated Show resolved Hide resolved
packages/cacti-weaver-driver-fabric/package.json Outdated Show resolved Hide resolved
packages/cacti-weaver-driver-fabric/package.json Outdated Show resolved Hide resolved
@sandeepnRES sandeepnRES force-pushed the feat_driver_pkg branch 2 times, most recently from 4e343e1 to 7ad535f Compare January 5, 2024 10:10
@outSH outSH self-requested a review January 17, 2024 13:53
@petermetz
Copy link
Member

Added package LGTM. Please add some descriptions as per my comments before merging though.

@sandeepnRES Big +1 on that. However trivial/straightforward it seems to us, it is always good to explain for future contributors/maintainers what the change is in a little more detail with some historical context, etc.

@outSH
Copy link
Contributor

outSH commented Jan 31, 2024

@sandeepnRES CI is failing, could you investigate? I think the package name is duplicated in both places and that's causing the scripts to fail

@outSH outSH disabled auto-merge January 31, 2024 11:23
@petermetz
Copy link
Member

@sandeepnRES Do you need any help investigating this one?

@sandeepnRES
Copy link
Member Author

sandeepnRES commented May 21, 2024

Hi @petermetz Sorry for the delay, this PR slipped our of my mind. What do you think will be better, to rename this package? Or to remove the one inside weaver directory from packages list declared in root package.json

@petermetz
Copy link
Member

petermetz commented May 24, 2024

Hi @petermetz Sorry for the delay, this PR slipped our of my mind. What do you think will be better, to rename this package? Or to remove the one inside weaver directory from packages list declared in root package.json

@sandeepnRES My vote goes for cacti-plugin-ledger-connector-fabric-weaver but I'm also open to others and flexible on the naming. I'd try to put it in the main directory for packages (e.g. the ./packages/ path) as well, but if it ends up being a huge extra work to move it around like that and you are in a rush to get it merged for whatever reason, I can also understand that (the guiding principle of mine here being that I do not want to hold up the progress with nit-picking too much)

The longer we keep pull requests open the more conflicts they get with main while being stuck in review so that's why I'm always trying to find sensible compromises just so that we can move things along at a decent velocity.

@sandeepnRES sandeepnRES force-pushed the feat_driver_pkg branch 2 times, most recently from 8e30685 to a774cc5 Compare June 17, 2024 06:03
@sandeepnRES
Copy link
Member Author

Hi @petermetz @VRamakrishna @outSH
Sorry for such a long delay, I've now renamed the package to cacti-plugin-weaver-driver-fabric
Haven't called it ledger connector yet, just to differentiate the original ledger connector and driver, once we combine both, then probably we can use that name.

@petermetz
Copy link
Member

@sandeepnRES OK, works for me. Please run yarn install to update the lock file.

> configure
> npm run init-registries && yarn install && yarn build:dev:backend


> init-registries
> npm config set @iroha2:registry=https://nexus.iroha.tech/repository/npm-group/

➤ YN0000: Yarn detected that the current workflow is executed from a public pull request. For safety the hardened mode has been enabled.
➤ YN0000: It will prevent malicious lockfile manipulations, in exchange for a slower install time. You can opt-out if necessary; check our documentation for more details.

➤ YN0000: · Yarn 4.1.0
➤ YN0000: ┌ Resolution step
Resolution step
➤ YN0000: └ Completed in 23s 737ms
➤ YN0000: ┌ Post-resolution validation
Post-resolution validation
➤ YN0028: +"@hyperledger/cacti-plugin-weaver-driver-fabric@workspace:packages/cacti-plugin-weaver-driver-fabric":
➤ YN0028: +  version: 0.0.0-use.local
➤ YN0028: +  resolution: "@hyperledger/cacti-plugin-weaver-driver-fabric@workspace:packages/cacti-plugin-weaver-driver-fabric"
➤ YN0028: +  languageName: unknown
➤ YN0028: +  linkType: soft
➤ YN0028: +
➤ YN0028: The lockfile would have been modified by this install, which is explicitly forbidden.
➤ YN0000: └ Completed in 0s 243ms
➤ YN0000: · Failed with errors in 24s 2ms
Error: Process completed with exit code 1.

@sandeepnRES sandeepnRES force-pushed the feat_driver_pkg branch 2 times, most recently from c379d2d to 244fbb7 Compare June 19, 2024 05:42
@sandeepnRES sandeepnRES enabled auto-merge (rebase) June 19, 2024 05:42
@sandeepnRES sandeepnRES force-pushed the feat_driver_pkg branch 5 times, most recently from 6f9768c to dabdcda Compare June 19, 2024 18:56
    fix(fabric-driver): yarn lint errors

Signed-off-by: Sandeep Nishad <sandeep.nishad1@ibm.com>
@sandeepnRES sandeepnRES merged commit 36b8470 into hyperledger:main Jun 19, 2024
149 of 150 checks passed
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.

None yet

4 participants