feat(ci)!: resolve the spec and scripts from the pinned workflow revision - #85
Closed
spydon wants to merge 1 commit into
Closed
feat(ci)!: resolve the spec and scripts from the pinned workflow revision#85spydon wants to merge 1 commit into
spydon wants to merge 1 commit into
Conversation
…iles The sync workflow's sdk-ref input defaulted to faba359, the commit that introduced the workflow back in #65. That looks like a pre-merge testing value that was never switched back, and it left the sync reading a July snapshot of capabilities/ while every validate workflow reads main. Any capability ID renamed since then therefore came back from the dead. #74 renamed or split five of them, so the last sync run opened supabase/supabase-flutter#1726 adding auth.sign_in.reset_password, realtime.channel.send, storage.file_buckets.list_files_paginated, storage.analytics.iceberg_namespace and storage.analytics.iceberg_table as not_implemented, and the validate job in the same pull request rejected all five as unknown feature IDs. Every SDK repo calling this workflow gets that same unmergeable pull request. Default to main and reuse the wording the validate workflows already have, so the two halves of the system agree on which spec is canonical.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Callers pin the reusable workflow by SHA, and then the workflow went back out to a branch for everything that actually matters. This makes the pin mean what it looks like it means.
The bug
supabase/sdk was resolved twice per run, and the two resolutions disagreed:
validate-sdk-compliance-{dart,javascript,python,swift}.ymlsdk-ref: main@mainsync-sdk-compliance.ymlsdk-ref: faba359a(20 July)faba359ais the commit that introduced the sync workflow in #65. It reads like a pre-merge testing value that was never switched back.Consequence one, the visible outage. The sync reads
capabilities/to find IDs an SDK'ssdk-compliance.yamldoes not mention yet, so a July snapshot resurrects everything renamed since. #74 renamed or split exactly five:auth.sign_in.reset_password→auth.sign_in.send_password_reset_emailrealtime.channel.send→realtime.channel.broadcaststorage.file_buckets.list_files_paginated→ merged intolist_filesstorage.analytics.iceberg_namespace→create_namespace/list_namespaces/delete_namespacestorage.analytics.iceberg_table→create_table/list_tables/load_table/update_table/rename_table/delete_tableThis morning's scheduled run added all five back as
not_implementedin all three repos that call the workflow, and the validate job in each of those very pull requests rejected them:Validate compliance fileunknown feature idUnmergeable by construction: each compliance file already carried the new names.
Consequence two, the supply chain hole. The four validate workflows loaded executable code from a branch (
sdk-compliance-validate,sdk-compliance-check-setup,sdk-compliance-check-symbols,sdk-compliance-check-drift, all@main), and read the spec and scripts frommainas well. Anything landing here reached every SDK's CI on the next run, with no pin to review and nothing to roll back to. It also means a rename in this repo breaks other repos' CI retroactively, which is the same defect as consequence one pointed the other way.The fix
Resolve everything from
github.job_workflow_sha, the commit SHA of the reusable workflow file itself. The caller's existing pin then governs the spec, the scripts and the actions alike, so there is exactly one ref in the system and it cannot skew. No branch is referenced anywhere.sdk-refsurvives as an explicit override for pre-merge testing, now defaulting to""and coalescing at the step, since input defaults are evaluated before the job context is populated.uses:accepts no expressions, so the composite actions cannot be referenced at that SHA directly. The spec is checked out first and they are loaded from it by path:Which is the pattern the docs describe for a reusable workflow that needs files co-located with its own definition.
That hoists the checkouts up out of
sdk-compliance-validateandsdk-compliance-check-setup, which lose their duplicated spec checkout and their now-unusedsdk-refinput. Nothing outside the four reusable workflows references these actions, so their contract change is contained.Ordering became load-bearing in the process.
actions/checkoutcleans the directory it manages, so where the calling repo occupies the workspace root it has to be checked out before_sdk-specor it deletes it; in thecheckjobs nothing takes the root, so the spec goes first because the actions are loaded from it. Both call sites carry a comment saying so.The
base-branch: maindefault in the sync workflow is untouched. That one is the caller's own branch to open a pull request against, not a ref anything is loaded from.Consumer impact
The spec becomes pinned rather than live, which is the point, but it is a behavior change worth stating plainly:
github-actions, so the bump arrives as a reviewable pull request without anyone doing anything.main. Bump, then sync.Marked
feat!rather thanfixso the changelog signals to the other SDK teams that the pinning semantics changed, and so the minor bump is a deliberate thing to read about.Verification
All 12 workflow and action files parse, and every
uses: ./_sdk-spec/...path resolves to a realaction.yml.No
@main,default: mainorsdk-ref: mainsurvives anywhere under.github/except the unrelatedbase-branchdefault.Ran the sync script against supabase-flutter's current
sdk-compliance.yamlwith the spec at this revision. It finds seven live IDs instead of the five dead ones, all seven resolving incapabilities/, which is the backlog from feat(realtime): add multiple postgres_changes filters capability #70, feat(storage): add the five missing Iceberg catalog capabilities #76 and feat: add six capabilities the registry was missing #78 that the sync should have been surfacing all along:The no-existing-group path also behaves:
storage.configuration.auto_retrylands under the script'splace manuallycomment,storage.configurationbeing a group feat: add six capabilities the registry was missing #78 added that no SDK carries yet.Still to do before merge, since none of the above exercises a real cross-repo call: point a throwaway supabase-flutter branch at this branch and confirm both jobs go green with
_sdk-specresolved fromjob_workflow_sha.