cacti icon indicating copy to clipboard operation
cacti copied to clipboard

build(sync-ts-config): integrating the weaver packages to the monorepo build

Open ruzell22 opened this issue 1 year ago • 5 comments

Commit to be reviewed


build(sync-ts-config): integrating the weaver packages to the monorepo build

Primary Changes
---------------

1. Relocated tsconfig file to cacti-plugin-weaver-driver-fabric
2. Added comment regarding the other package not having tsconfig and will be ignoring till then

Fixes: #3366 Related to: #3069

Pull Request Requirements

  • [x] 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.
  • [x] 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.
  • [x] Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

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

A Must Read for Beginners For rebasing and squashing, here's a must read guide for beginners.

ruzell22 avatar Jul 11 '24 06:07 ruzell22

@petermetz I have updated my branch and repush this PR. I can't see the weaver CI job that is failing. I have also tried to look into past latest commit in the main repo and looked for weaver that are failing and didn't see one. I would like to know the specific weaver job that is failing on your end, thank you

ruzell22 avatar Jul 15 '24 08:07 ruzell22

@ruzell22 I think Peter meant the following (required) CI jobs that are failing:

  • Test All Docker Images Build / build_docker_fabric_driver_local (pull_request)
  • Test Asset Transfer / asset-transfer-local (pull_request)
  • Test Data Sharing / data-sharing-docker-local (pull_request)
  • Test Data Sharing / data-sharing-local (pull_request)

Take a look at CI jobs listing on the bottom of this PR to see the details and logs why these jobs are failing

outSH avatar Jul 15 '24 09:07 outSH

The folders added to the ignore list are modules and sample apps that are used in the Weaver test harnesses (see list posted by Michal above).

I'm not sure what the purpose of this PR is. Could you clarify, @ruzell22 ?

Is there a requirement for TypeScript packages to have a tsconfig.json, @petermetz ?

We are undertaking an integration of Weaver packages into the Cacti monorepo, and one of our mentees (Jennifer Bell) is addressing part of that process. The cacti-weaver-driver-fabric package was sort-of integrated into the monorepo as an experiment, whereby a shell package was created in the packages folder with a symlink to the actual source code within the weaver folder. We plan to do something similar for other packages as a stopgap measure before a more comprehensive integration (as per our envisioned architecture, see ROADMAP.md) is done, which will take a while.

But whether or not a package contains a tsconfig.json or not shouldn't matter IMO.

@ruzell22 If you are interested in exposing all other Weaver packages the way I described above for the Fabric Driver, be my guest. @sandeepnRES and I can advise you on that and you can submit a comprehensive PR then.

VRamakrishna avatar Jul 16 '24 10:07 VRamakrishna

Is there a requirement for TypeScript packages to have a tsconfig.json, @petermetz ?

@VRamakrishna It's not. Longer term, I'd love it if it was, but because I also don't want to slow down teams working on code who aren't there yet (as in not ready to have Typescript instead of Javascript).

The vision: It would be a nice bump for developer experience if the Typescript compiler was checking all NodeJS/browser source code. It makes entire classes of bugs impossible (not nearly all of them, but small wins are good too).

But whether or not a package contains a tsconfig.json or not shouldn't matter IMO.

The reason why it matters is because the automation is currently broken (it assumes that there is a tsconfig.json for all nodejs packages in the monorepo). So we definitely need that portion of the PR that makes it so that certain directories are ignored by the script. The part of the diff that moves around the tsconfig.json file that's in the weaver package can be removed (so that the PR only makes the change to the script under ./tools/ and then we can continue the discussion about the integration tasks in another PR/issue thread.

Please see my comment in the thread above. I'd recommend closing this PR and working with us on a separate PR if you are interested in carrying out integration tasks.

@VRamakrishna Have you seen this thread? https://github.com/hyperledger/cacti/issues/3366#issuecomment-2216674164


@ruzell22 Please remove the part of the diff that moves around the tsconfig.json file so that it only changes ./tools/ and that way we can simplify the review process. Please also make sure to update the ignore patterns as necessary so that the sync ts config script completes successfully. After making those changes please re-request review from everyone to make sure.

petermetz avatar Jul 18 '24 03:07 petermetz

@petermetz I have updated my repo and remove the moving of tsconfig.json. After re-running the yarn run sync-ts-config , the updated ignore patterns are the same as the one in #3367 that has been merged already. I guess this ticket and the PR can be marked as close since it will not make changes in code after the updated ignore patterns image

ruzell22 avatar Jul 19 '24 03:07 ruzell22

@petermetz Let's close this as @ruzell22 asked for, unless you see a reason to merge it. As @sandeepnRES says above, this doesn't do anything other than change a whitespace.

VRamakrishna avatar Sep 04 '24 13:09 VRamakrishna