-
Notifications
You must be signed in to change notification settings - Fork 14
POC: Configure ipa-validation-ruleset package #840
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Lovisa Berggren <[email protected]>
sparse-checkout: | | ||
openapi/ | ||
tools/spectral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this checkout?
runs-on: ubuntu-latest | ||
name: Check Semantic Commits | ||
steps: | ||
- name: Checkout repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[FYI] Checkout of the repository is optional step if your action does not need repository (uses GH API you can skip it)
push: | ||
branches: | ||
- main | ||
- CLOUDP-329998 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to remove it before merge
github_token: ${{ secrets.GITHUB_TOKEN }} | ||
branch: main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Unsure if that will work (we do have branch protection rules)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we get changelog generated in separate PR.
You need specific filters and standards for changelog to be introduced for it to have any value.
@@ -0,0 +1,25 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have a root package.json, we need to configure npm workspaces by adding:
{
"workspaces": [
"path/to/subfolder"
]
}
As General FYI: A nested package.json without proper workspace configuration can cause:
- Publishing conflicts - npm may not recognize the correct package scope
- Dependency resolution issues - nested dependencies might not resolve properly
- NPM metadata - npm/tools may not know which package.json to use as the source of truth
- Missing Dependencies/Scripts Migration
My suggestion is to split this PR and start with proper package structure.
PR is missing certain aspects:
First PR: Set up proper npm workspace structure
Configure workspaces in root package.json
Move/duplicate necessary dependencies
Ensure scripts work in workspace context
Test locally with npm pack
Second PR: Enable Dependabot after confirming the package structure works
Third PR: Add semantic commits checks
... etc. etd.
This approach will allow us to validate and review the package structure before introducing automated semantics checks and publish, reducing the risk of broken publishes to NPM that will require lengthy support tickets.
@@ -2,6 +2,20 @@ | |||
|
|||
The IPA validation uses [Spectral](https://docs.stoplight.io/docs/spectral/9ffa04e052cc1-spectral-cli) to validate the [MongoDB Atlas Admin API OpenAPI Specification](https://github.com/mongodb/openapi/tree/main/openapi). The rules cover MongoDB's [Improvement Proposal for APIs](https://mongodb.github.io/ipa/) (IPA), which are best-practices for API design. | |||
|
|||
|
|||
## Quick Links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you plan for this readme to be part of the NPM docs then your package is missing dependencies.
@@ -32,7 +33,6 @@ | |||
"@stoplight/spectral-ruleset-bundler": "^1.6.3", | |||
"apache-arrow": "^21.0.0", | |||
"dotenv": "^17.2.0", | |||
"ember-inflector": "^6.0.0", | |||
"eslint-plugin-jest": "^29.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of those are actually dev dependencies and not runtime dependencies.
We need to have minimal set of dependencies for npm package as it will be installed without dev dependencies
cache: 'npm' | ||
- run: npm ci | ||
- run: npm test | ||
- uses: JS-DevTools/npm-publish@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security recommendation. Do not use tag for third party packages that deal with credentials.
Edit:
You could avoid using action and have a separate publish script etc.
- uses: JS-DevTools/npm-publish@v3 | |
- uses: JS-DevTools/npm-publish@sha here | |
with: | |
token: ${{ secrets.IPA_VALIDATION_NPM_TOKEN }} | |
package: "tools/spectral/ipa/package.json" |
- main | ||
- CLOUDP-329998 | ||
paths: | ||
- 'tools/spectral/ipa/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Do we want to publish on non code changes like Readme.md
Summary: @sphterry Thank you for amazing changes covering everything we need to publish package.
|
Proposed changes
Support for IPA Validation Framework NPM package.
More details
Jira ticket: CLOUDP-329998