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

chore(plugins): unify package json fields #3714

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Jan 3, 2025

  • Unify common fields among all cacti plugin package.json files.
  • Use Cacti instead of Cactus whenever possible.
  • Use current github URL and mailing list addresses.
  • Add publishConfig access public to all packages.

Prerequisite for #3670 where I plan adding a CI check that will assert that all package.json files are correct.

I'm using the following Joi schema as our standard:

const schema = Joi.object({
  name: Joi.string()
    .pattern(new RegExp("^@hyperledger(-cacti)?/.*"))
    .required(),
  version: Joi.string().valid("2.1.0").required(),
  private: Joi.bool().valid(false),
  description: Joi.string().min(10).required(),
  keywords: Joi.array()
    .items(Joi.string())
    .has(Joi.valid("Hyperledger"))
    .has(Joi.valid("Cacti"))
    .required(),
  homepage: Joi.string()
    .valid("https://github.com/hyperledger-cacti/cacti#readme")
    .required(),
  bugs: Joi.object()
    .valid({
      url: "https://github.com/hyperledger-cacti/cacti/issues",
    })
    .required(),
  repository: Joi.object()
    .valid({
      type: "git",
      url: "git+https://github.com/hyperledger-cacti/cacti.git",
    })
    .required(),
  license: Joi.string().valid("Apache-2.0").required(),
  author: Joi.object()
    .valid({
      name: "Hyperledger Cacti Contributors",
      email: "cacti@lists.lfdecentralizedtrust.org",
      url: "https://www.lfdecentralizedtrust.org/projects/cacti",
    })
    .required(),
  files: Joi.array().items(Joi.string()), // required? dist?
  engines: Joi.object()
    .valid({
      node: ">=18",
      npm: ">=8",
    })
    .required(),
  publishConfig: Joi.object({ access: Joi.string().valid("public") })
    .unknown()
    .required(),
}).unknown();

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).

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

@outSH
Copy link
Contributor Author

outSH commented Jan 3, 2025

@petermetz As mentioned in the commit messasge, this is part of #3670 where I plan to add common package json fields check. I've followed your recommendation to use Joi, please have a look at proposed schema and let me know if you'd like to change / add anything to it. I also have the following questions:

  • I use version check that is hardcoded in a test file (will move it a a const to the top of the file to make it easier to change). Would you like another solution that will cause less troubles during release? Environment variable, maybe add version field to the top package.json file that will be used as a reference when verifying the child packages? I don't want this to be yet another burden for you during release :)
  • I wanted to require that all packages (at least in packages/) are set to public (i.e. not private: true) but I found there is package that break it (cactus-test-plugin-keychain-memory). Shall we change it to public, move it to another directory, or leave as it is and don't check for private in the CI check? I assume it's OK to have private packages in examples/

@VattiPraveen @sandeepnRES I've added...

  "publishConfig": {
    "access": "public"
  },

... to your package.json files in weaver, is that OK? I think that was required by @petermetz at least for cacti-native packages at some point (if I recall correctly), but I'm not sure if it doesn't affect weaver release process in any substantial way. I don't think it should, but please confirm it before the merge so I don't mess up your build by accident.

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.

LGTM
@petermetz when are we planning to change the package names themselves? (from cactus-cmd-api-server to cacti-cmd-api-server...)

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.

@outSH Very sorry for the slow response and review! LGTM overall but I'm omitting the approval because Jagpreet already approved it from our organization.

To answer the specific questions:

  1. If you could parse it out of either the lerna.json or root package.json I would be forever grateful. That way it's completely on auto-pilot instead of having any manual steps at all. I really appreciate the consideration for the reduction of manual release steps, thank you very much for checking in about this! A good example of how easy it can be to pull the version data from lerna.json is shown in this file: tools/custom-checks/check-package-json-sort.ts where I do an import like this:
import lernaCfg from "../../lerna.json" assert { type: "json" };
  1. I would say enforce for all packages that are set to public that are in the scope of the lerna/yarn workspace. So in other words, if the array of blogs in lerna.json matches a package then it should be checked by the CI for it to be set to public.
    Currently the array of glob looks like this so both the test package you mentioned, the example/* packages and the weaver packages are captured by it and therefore my ask would be to check all of them for ensuring that it is set to public:
"packages": [
    "packages/cactus-*",
    "examples/cactus-*",
    "extensions/cactus-*",
    "packages/cacti-*",
    "examples/cacti-*",
    "extensions/cacti-*",
    "weaver/common/protos-js",
    "weaver/sdks/fabric/interoperation-node-sdk",
    "weaver/sdks/besu/node",
    "weaver/core/drivers/fabric-driver",
    "weaver/core/identity-management/iin-agent",
    "weaver/samples/fabric/fabric-cli",
    "weaver/samples/besu/besu-cli",
    "weaver/samples/besu/simpleasset",
    "weaver/samples/besu/simplestate"
  ],

If someone really wants to have their package private then they should remove it from the lerna.json glob array so that the release process doesn't crash due to us trying to release a private package. (but then they are also excluded from all script executions and other CI checks so to be honest I would advocate strongly against this workaround and instead just considering that all of the code is out on the internet anyway so what's the point of having a private setting in the publish config)


Overall, thank you again, this is an awesome improvement to the CI!

@petermetz
Copy link
Contributor

LGTM @petermetz when are we planning to change the package names themselves? (from cactus-cmd-api-server to cacti-cmd-api-server...)

@jagpreetsinghsasan As soon as possible. My plan is to stabilize the release automation first just so that we have the ability to reliably and with low effort issue patch and minor releases for all the packages.

@petermetz
Copy link
Contributor

@VattiPraveen @sandeepnRES I've added...

"publishConfig": {
"access": "public"
},

... to your package.json files in weaver, is that OK? I think that was required by @petermetz at least for cacti-native
packages at some point (if I recall correctly), but I'm not sure if it doesn't affect weaver release process in any substantial > way. I don't think it should, but please confirm it before the merge so I don't mess up your build by accident.

@outSH Not sure but I'm guessing you wanted to cc @VRamakrishna and @sandeepnRES there.

@outSH
Copy link
Contributor Author

outSH commented Feb 17, 2025

@petermetz Thanks for the info! I've changed all packages from lerna to public as suggested, the CI will enforce it as well.

@VRamakrishna and @sandeepnRES Please review and respond to my previous concern, that is:

I've added...

  "publishConfig": {
    "access": "public"
  },

... to your package.json files in weaver, is that OK? I think that was required by @petermetz at least for cacti-native packages at some point (if I recall correctly), but I'm not sure if it doesn't affect weaver release process in any substantial way. I don't think it should, but please confirm it before the merge so I don't mess up your build by accident.

- Unify common fields among all cacti plugin package.json files.
- Use Cacti instead of Cactus whenever possible.
- Use current github URL and mailing list addresses.
- Add publishConfig access public to all packages.

Prerequisite for hyperledger-cacti#3670 where
I plan adding a CI check that will assert that all package.json files are correct.

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
@outSH outSH force-pushed the unify_package_json_files branch from 36ce72d to d0bd21b Compare February 17, 2025 12:06
@jagpreetsinghsasan
Copy link
Contributor

Fixes #3666

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.

3 participants