-
Notifications
You must be signed in to change notification settings - Fork 177
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
Add upload to registry workflow for foundry_std #2461
base: master
Are you sure you want to change the base?
Conversation
177666e
to
6e31415
Compare
Blocked by software-mansion/scarb#1605 |
<!-- Reference any GitHub issues resolved by this PR --> Related to #2344 This PR is a part of the stack: --> Add missing info recommended for package uploads (This PR) -- Add github action workflow for automatic registry uploads during release (#2461) ## Introduced changes <!-- A brief description of the changes --> - Adds readmes and links to respective Scarb.tomls, so to view this metadata on the registry and supress scarb warnings - Bump snforge_scarb_plugin version to `0.31.0`, to properly reflect its version in regards to the registry. **From now on all changes made to this crate should be concluded with version bump** ## Checklist <!-- Make sure all of these are complete --> - [X] Linked relevant issue - [X] Updated relevant documentation - [X] Added relevant tests - [X] Performed self-review of the code - [X] Added changes to `CHANGELOG.md` --------- Co-authored-by: Fiiranek <jellybean@onet.eu> Co-authored-by: Franciszek Job <54181625+franciszekjob@users.noreply.github.com>
This reverts commit da4a5b8.
It needs clarification, but plugins weren't correctly handled by edit: The |
It shouldn't have anything to do with uploading packages though |
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.
Partial review, please re-request when ready 👍🏻
# todo: Use scarb stable version that support publishing plugins (after 2.8.4) | ||
scarb-version: "nightly-2024-11-09" | ||
|
||
- name: Check if package version exist in the registry |
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.
Is this step necessary?
We implemented this for publishing Scarb plugins because their version matches the Cairo version. Since multiple Scarb versions can correspond to a single Cairo version, running the workflow with every Scarb release could cause errors by publishing plugins with the same version.
Afaik, this doesn’t apply to snfoundry plugins because their version isn’t tied to any Cairo version. Normally, the workflow should only be triggered once per tag.
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.
We might decouple foundry plugin version from foundry itself so it might be the case that for consecutive foundry releases, plugin version will stay the same.
So we have to make sure we don't reupload the plugin to the registry in these cases.
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.
Sometimes we patch plugin, without release then we would like to publish it and sometimes we will not do release plugin together with snforge_std and sncast_std, we want to set it on 1.0.0 and keep it until there are changes, and then it would fail the workflow and CI will be red at the 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.
As I pointed out offline, if you're going in the direction of decoupling plugin from foundry itself, it seems to me it a separate workflow should be considered
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 is one of the opportunities, but isn't it good enough? @cptartur
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.
Similarly to @DelevoXDG , I would probably be in favour of splitting release flows for macros and standard libraries as well.
My understanding is that the plugin won't necessarily follow the *_std version, so it may make sense to release it independently. For this manual workflow dispatch only is good enough. Most of logic from this file is fine, though you wouldn't need anything from this step (I mean the check-versions, which I find very hacky) so I'd actually argue it could become far simpler.
Having this, we could probably incorporate *_std upload to the release.yml
workflow, as it seems to me this is more natural place for this. We already verify the version there, so chances are we do not need any further modifications - we could probably just add a new job (standard-libraries-release, or scarbs-release or sth) that will set the plugin version (this is unnecessary if you decide to set the version in Scarb.toml in repo) and use scarb to publish and call it a day. No need to check versions against the registry, verify what should/should not be published etc.
Thoughts?
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 generally agree with you, but I can still see a few problems we could encounter. In scenario where this workflow is splited and moved to release
we can get error while releasing new versions of _std and plugins simultaneously. Then plugin should be then uploaded before _std, to make snforge_std
dependency working. As other job in can be made sequential, but if we like to provide workflow_dispatcher then its generates a lot of unnecessary boilerplate code (ifs
) in entire workflow, because we can’t actually specify exact job to run on dispatch. In scenario of two different workflows it actually can’t be made sequential and then we can only hope that plugin will be uploaded first.
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.
In scenario where this workflow is splited and moved to release we can get error while releasing new versions of _std and plugins simultaneously
Well I do not think we necessarily need to release macro in the same time we release package, and because of that workflow_dispatch would be ideal. I can easily imagine macro not being updated for months, or be updated multiple times during one week when we do not want to release new snfoundry version.
If you wanted to the release workflow could call the plugin publish workflow, to have the functionality we're aiming for and be sure that the release operations are indeed sequential.
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 you wanted to the release workflow could call the plugin publish workflow, to have the functionality we're aiming for and be sure that the release operations are indeed sequential.
That's how I see it as well, we should have:
- Plugin publish workflow with
workflow_dispatch
andworkflow_call
- Foundry publish workflow on
release
andworkflow_dispatch
. It should first should check if plugin is up to date in first job and re-use the first workflow if necessary to upload the plugin, and release the foundry libs afterwards
As other job in can be made sequential, but if we like to provide workflow_dispatcher then its generates a lot of unnecessary boilerplate code (
ifs
) in entire workflow, because we can’t actually specify exact job to run on dispatch.
Do you have any specific examples of why that might be the case? 🤔 I don't think this should really cause any boilerplate or significantly more ifs than we have now
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.
Seems convincing. Updated.
# todo: Use scarb stable version that support publishing plugins (after 2.8.4) | ||
scarb-version: "nightly-2024-11-09" | ||
|
||
- name: Check if package version exist in the registry |
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.
We might decouple foundry plugin version from foundry itself so it might be the case that for consecutive foundry releases, plugin version will stay the same.
So we have to make sure we don't reupload the plugin to the registry in these cases.
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.
@THenry14 Can you please review as well? I know it's technically your PR but it's changed a bit 😅
# todo: Use scarb stable version that support publishing plugins (after 2.8.4) | ||
scarb-version: "nightly-2024-11-09" | ||
|
||
- name: Check if package version exist in the registry |
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.
Similarly to @DelevoXDG , I would probably be in favour of splitting release flows for macros and standard libraries as well.
My understanding is that the plugin won't necessarily follow the *_std version, so it may make sense to release it independently. For this manual workflow dispatch only is good enough. Most of logic from this file is fine, though you wouldn't need anything from this step (I mean the check-versions, which I find very hacky) so I'd actually argue it could become far simpler.
Having this, we could probably incorporate *_std upload to the release.yml
workflow, as it seems to me this is more natural place for this. We already verify the version there, so chances are we do not need any further modifications - we could probably just add a new job (standard-libraries-release, or scarbs-release or sth) that will set the plugin version (this is unnecessary if you decide to set the version in Scarb.toml in repo) and use scarb to publish and call it a day. No need to check versions against the registry, verify what should/should not be published etc.
Thoughts?
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.
great job mate
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 👍🏻
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.
Closes #2344
Closes #2463
This PR is a part of the stack:
-- Add missing info recommended for package uploads (#2460)
--> Add github action workflow for automatic registry uploads during release (This PR)
Introduced changes
Checklist
CHANGELOG.md