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

POC Github Actions update fides version in fidesplus #5874

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

JadeCara
Copy link
Contributor

@JadeCara JadeCara commented Mar 12, 2025

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

  • added a new github workflow to auto new tags on pr merge.
  • added a new github workflow to auto generate prs for new tags.

Steps to Confirm

  1. Create a PR (maybe update a doc) and merge into fides
  2. wait for CI to run
  3. Verify that this creates a new b tag
  4. Verify new PR appears in fidesplus with the correct tagged version.
    Screenshot 2025-03-12 at 11 07 01 AM

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Copy link

vercel bot commented Mar 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 20, 2025 5:54pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Mar 20, 2025 5:54pm

@JadeCara JadeCara marked this pull request as ready for review March 12, 2025 17:11
@JadeCara JadeCara changed the title POC to see if this works the way I remember POC Github Actions update fides version in fidesplus Mar 12, 2025
Copy link
Contributor

@adamsachs adamsachs left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

Copy link
Contributor

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 👍

Copy link
Contributor

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

2 participants