-
Notifications
You must be signed in to change notification settings - Fork 80
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
POC Github Actions update fides version in fidesplus #5874
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
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.
@JadeCara thank you for being proactive on this, and sorry it's taken me so long to get to a review!
generally i think this shows some promise, and i think it's probably worth us trying something like this out. see my comments for some improvements/tweaks - the one about sending a dispatch event in particular is one that i'd like to see implemented before we try this out.
beyond this, the only broader concern i'd have is that we'll significantly increase the number of beta tagged packages that we publish to pypi. i do believe there are some storage limits on pypi - i'm not sure if we're anywhere close to running into them (we have run into storage limits on test pypi, where we host all of our packages built from alpha tags/commits to main). that's something we can try to monitor and keep our eyes on, i don't think it needs to be a blocker.
as i say that, it makes me realize that we'll probably want to update our existing workflow for just general commits to main
. currently we do publish a package to test pypi on every commit to main (e.g. see here), and these are used in 'double-edge' builds in fidesplus. the problem is/has been that CI is not run against those packages! so that'd overlap with what you've got here - so if we want to move forward with what you've got here, we should probably rip all that out...
there are some pros/cons to weigh here, it may be worth a bit further of a discussion offline, my thoughts now have expanded beyond the scope of this PR!
on: | ||
push: | ||
tags: | ||
- '*b*' # Runs on only beta tag 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.
nice 👍
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.
hmm, it feels odd to me that we're actually editing the fidesplus repo here from an action defined in the OSS repo. especially since fidesplus is closed source while fides is OSS, even though i don't think non-org users can do much with our workflows, so i don't think it's technically a security risk. it still feels improper to me, though.
i think a better approach would be to lift this workflow into the fidesplus
repo, define it there, and have it triggered via an event that's dispatched via your create_tag
workflow above here in fides. we've got quite a few examples (see here for one) of something like this to coordinate actions across repos 👍
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.
thinking on it more - the better place to put the dispatch event to trigger creation of a fidesplus PR is likely actually in the publish_package.yml
job that actually handles publishing the beta tag package to pypi - and we've already got some dispatch events being triggered there specifically on release/rc/beta tags, which i think is what we'd want to do here (or maybe we do just want to limit to beta tags, that's fine).
this will ensure that the PR is opened once the package actually is on pypi - which is what's needed if we're going to reference it in the requirements.txt
of fidesplus!
Co-authored-by: Adam Sachs <adam@ethyca.com>
Description Of Changes
Adds a github action that will auto generate tags when a PR is merged into fides.
Adds a github action that will automatically create a PR in fidesplus updating the version of fides to the new tag.
Code Changes
Steps to Confirm
b
tagPre-Merge Checklist
CHANGELOG.md
updatedmain
downgrade()
migration is correct and works