-
Notifications
You must be signed in to change notification settings - Fork 31
INTPYTHON-821 Add SBOM update automation #451
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
|
Need to update this to use cyclonedx-py plugin instead. Will move to ready review once finish. |
aclark4life
left a comment
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.
LGTM!
|
You write that "This triggers on changes to any of the requirements and toml files on the master branch.", however, it's also obsoleted whenever a new version of pymongo or Django is released that matches the requirements. For example, we have the dependency "django>=5.2,<6.0" and generally a new version of Django 5.2.x is released each month. Does the SBOM need to be updated each time? |
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.10" |
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.
Any reason not to use the latest version of Python?
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 think the convention (in PyMongo at least) is to use the min, so right now we're dropping 3.9 and adding 3.10 in various places. I don't think we necessarily need to do that here but I also don't know if we gain anything by switching 3.13 or 3.14.
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.
For tests, I understand. But this is just a utility script. The Python version won't affect the output, right? My thought was that if we use a later version we won't have to update it as often.
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 main reason for pinning is if the team ever plans to use older version of libraries that new version of python might not support. If this isn't a problem latest would be better like you said.
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, we wouldn't use such libraries.
| paths: | ||
| - 'pyproject.toml' | ||
| - 'requirements.txt' |
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.
It's possible for these files to have changes that don't involve any dependency changes. Is there logic in this job that prevents a PR from being opened in this case? (I guess GitHub may disallow a PR with no changes, or possibly it would be closed immediately; just want to confirm.)
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'm not sure myself, but I would say that if it:
- Has no changes, it wouldn't populate
- Does make a new one, we just close it. The SBOM generation matters most right before we push a 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.
Depending on the answer to #451 (comment), it might be better to simply make this part of the pre-release process.
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 the file changes at all, it will trigger the action to run and make an sbom PR. you'd just see the dates/metadata of the sbom.json to be changed. In that scenario the team can just close the PR.
Our team does require that the master branch have an up-to-date sbom for tracking. I can definitely add the logic if you think it's worth the extra step (actions will still get ran regardless, but we'd just compare the sbom files to see if any components actually got changed)
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 it's straightforward, I think it would be valuable to avoid useless PRs, especially if this script has to be applied to many repositories.
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.
And incidentally, we don't have any dependencies in pyproject.toml (besides the optional ones for building the docs which your script doesn't install, so won't be detected anyway) so do we need to have this job watch that file?
The SBOM doesn't become stale when new patch versions are released. The SBOM captures the minimum required versions from the dependency ranges in requirements.txt (e.g., Django 5.2 from django>=5.2,<6.0), not whatever latest version happens to be available. The workflow triggers only when dependency specifications actually change in the requirements files (based on what runtime users would install). When release branches are cut, it should automatically snapshot the sbom.json from main at that point in time, which ships with that release. It does become stale in release branch if you are making one off requirements change in those branch though if that's what you're referring to? If that's the case then those branch sbom would need to be updated as well. |
This doesn't seem true. In thanhnguyen-mdb#4, I see a refernce to "Django==5.2.8" which was the latest Django release at the time that job ran.
You'll have to explain more about how this works. Our release process doesn't use release branches. Currently, the main branch corresponds to Django MongoDB Backend 5.2.x which works with Django 5.2.x. We have for older versions, e.g. the 5.1.x corresponds to Django MongoDB Backend 5.1.x and Django 5.1.x. |
Sorry, got info a bit mixed up. It's the minimum version/installed version when a user runs pip install -r requirements.txt (runtime version for their python version).
Ah that makes more sense then! If I make it trigger a PR on master/release branch (5.1.x, etc), would that suffice then? This would be a bit more PR but will keep all branch updated. Welcome to suggestions here. I'll add extra logic to avoid false positive PR in the meantime. |
|
Here's how I foresee this script working:
It seems very arbitrary to make these sort of updates based on when changes to these files are made. Do you see my point? |
It doesn't seem correct. An update to the dependencies on the main branch doesn't mean they changed on other branches. |
INTPYTHON-821
Summary
Added in Github action workflow for SBOM automation. This triggers on changes to any of the requirements and toml files on the master branch. A new branch will be created for the PR and closed on merge.
Sample PR for the SBOM: thanhnguyen-mdb#4
Changes in this PR
New sbom.yml file for the Github action workflow
Testing Plan
Tested through Github Action triggers
Checklist
Checklist for Author
Checklist for Reviewer {@primary_reviewer}