-
Notifications
You must be signed in to change notification settings - Fork 14
feat(ipa): IPA Package config, docs and release workflow #855
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
Conversation
@@ -0,0 +1,324 @@ | |||
# Contributing to MongoDB IPA Validation |
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.
Can you flag the new content? I do not see release procedure present.
In future consider creating tiny PRs that move content to separate them from content creation.
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.
Lines 71-77. Thank you for flagging, I'll keep that in mind for the future
Release Workflow UnderstandingProcess Overview:
Key Assumptions:
Questions for Review:
|
tools/spectral/ipa/.npmignore
Outdated
@@ -0,0 +1,3 @@ | |||
metrics |
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 is a file in metrics collector.js
that is used to collect exceptions, adoptions, violations when running the IPA validation rules, I think we need to include this in the npm package. Perhaps we can move it into the helper functions instead, and keep metrics to contain things not needed for the spectral run
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.
No need to move it (or if there is, we can do it in a separate PR). I just change the .npmignore to keep that file in the package. Retested it with npm pack
and it works👍
In MCP (packaged as npm package) instead of comparing two versions we just check if the current version exists as a git tag, if not proceed with deploy (https://github.com/mongodb-js/mongodb-mcp-server/blob/0083493a5bf5afeca84be6fd2813ca4060347fad/.github/workflows/publish.yaml). Just FYI, not asking to make any changes at all, I just think it is easier to follow if the check is in the workflow and the execution of the action lives within scripts. |
tools/spectral/ipa/CONTRIBUTING.md
Outdated
|
||
#### For IPA Package version bump | ||
|
||
A new version of the IPA package will be released when the version is bumped. Before merging, please run |
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.
A new version of the IPA package will be released when the version is bumped. Before merging, please run | |
A new version of the IPA package will be released when the version in package.json is changed. Before merging, please run |
tools/spectral/ipa/CONTRIBUTING.md
Outdated
``` | ||
npm run gen-ipa-changelog | ||
``` | ||
and commit the changes. The changelog must only be updated alongside a version bump. |
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.
Can we check that as part of the release job?
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.
Alternatively what I used to do is to run job from tag:
- User creates tag/release.
- Job runs to create PR with version change and change log.
- Once merged package is published and changelog in release notes updated
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.
The changelog is checked on pull requests by .github/workflows/ipa-changelog.yml
after that given pull request is merged, it triggers the release workflow. I didn't know about tags until these PR comments! I wish I'd known sooner
tools/spectral/ipa/CONTRIBUTING.md
Outdated
|
||
- [ ] Reference related issues (e.g., Closes #123) | ||
|
||
#### For IPA Package version bump |
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 would expect docs covering how package is released.
Version bump is technical implementation
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've updated it to be a separate checklist, let me know if I missed anything
tools/spectral/ipa/CONTRIBUTING.md
Outdated
A new version of the IPA package will be released when the version in the package.json is changed. To release a new version: | ||
|
||
- [ ] Determine whether your update is [major/minor/patch] following [semantic versioning](https://semver.org/) | ||
- [ ] Update the version number in package.json | ||
- [ ] Run `npm run gen-ipa-changelog` and commit the changes. | ||
- [ ] Open a PR and ensure the title is conventional and scoped to IPA (ie: `ci(ipa): new version`) |
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: Some wording/intentions changed. Reordered as we say people to bump package and then generate changelog where we need changelog to figure out how to bump package
A new version of the IPA package will be released when the version in the package.json is changed. To release a new version: | |
- [ ] Determine whether your update is [major/minor/patch] following [semantic versioning](https://semver.org/) | |
- [ ] Update the version number in package.json | |
- [ ] Run `npm run gen-ipa-changelog` and commit the changes. | |
- [ ] Open a PR and ensure the title is conventional and scoped to IPA (ie: `ci(ipa): new version`) | |
Release process requires generation of changelog and manual update of version in package.json. | |
A new version of the IPA package will be released when the version in the package.json is changed. | |
Process: | |
- [ ] Run `npm run gen-ipa-changelog` and commit the changes. | |
- [ ] Based on changelog determine whether your update is [major/minor/patch] following [semantic versioning](https://semver.org/) | |
- [ ] Update the version number in package.json | |
- [ ] | |
- [ ] Open a PR and ensure the title is conventional and scoped to IPA (ie: `ci(ipa): package release`) |
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.
This is a good point, I made the assumption here that a version bump will have been reviewed internally given commit history. I will add this nit to fix in our documentation changes ticket
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 made the assumption here that a version bump will have been reviewed internally given commit history.
IMHO Author owns the bump decision. Otherwise you would have all reviewers duplicating the work of author to validate version bumps
@fmenezes To give context, tags have been rejected for now since they are being used for the FOASCLI release workflow and we want to avoid conflicts. Definitely refinement to consider in the future, but outside the scope of the current epic. Thank you for flagging, I've added a section to the tech design. |
Proposed changes
npm pack
Note: Follow-up ticket CLOUDP-334400 to separate legacy Spectral ruleset into it's own directory with distinct documentation
Jira ticket: CLOUDP-329998