-
Notifications
You must be signed in to change notification settings - Fork 78
fix: improve beta release cicd #202
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
Conversation
d3b3c02 to
70d2c38
Compare
15b6be3 to
2c95b9a
Compare
a9e8011 to
3efd4cf
Compare
6dec41d to
f1aec99
Compare
jirispilka
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.
IMO, this has a lot of custom logic, and I’m not sure we really need to add it here. I think we should aim for as unified a deployment as possible across apify packages
I checked few Apify repose and it seems that this beta release workflow is not centralized and each repo handles that separately. I think developer flexibility is important and thus would also keep this decentralized, per repo. These repos use a variant of
These repos use on push to master model:
In the end I think this label solution is the best compromise, if you need that you can just tag and it does not run on each push to master when it's not needed. Also there is a lower chance that it breaks the versioning, last time for some reason the beta version jumped straight from stable 0.2.15 to 0.3.1-beta.0 for some reason 🤷♂️ |
|
Let’s ask @janbuchar or @B4nan for their opinion and review. Purpose: We’d like to release a beta version of this package so we can include it at |
MichalKalita
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.
I don't fully understand GitHub yaml configs, but JS part looks good.
|
So what exactly are you trying to solve here? Do you need canary builds for PRs, or for stuff in master? Normally, we just run the canary builds for every commit to master, making this dependent on reopening a merge PR feels quite weird to me (that's what this PR is doing, right?)
This doesn't sound right to me, first of all, I really dislike the idea of git tags for every commit to master, I'd just drop those - and base the next vesion only on NPM, that is the single source of truth to me. Also, I would prefer checking the version from NPM API instead of package.json |
|
This is what we use in crawlee for detection of the next canary version number: |
The Apify MCP is spread across two repos, this one and the I included the git tag logic so there is a second source of truth, since one time the beta release script cause this:
We can remove the git tag logic and add more logging so we can then trace what causes this version jump if that happens again for some reason. |
|
I see, we don't really deal with releases for unmerged PRs. I recently came across https://pkg.pr.new/, maybe you could give that a try. Note that its not a great idea to try to come up with some ordered versioning outside of the master branch, one PR will do something, the other will do something else, they won't share the code, so better to keep their versions unrelated. |
|
Thanks, guys. IMO, https://pkg.pr.new/ looks like exactly what we need to create an ad-hoc release for testing |
commit: |
|
Thank you @B4nan for recommending the https://pkg.pr.new/, it works super well 👍 |
|
Great to hear that, we'll probably adopt it too in other projects if it worked well for you guys. |
jirispilka
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.
Really cool that it’s working!
I think it deserves a bit of documentation in the README. If it’s something others might adopt, it would be great to briefly document why we’re using it (what problem it solves) and how it works.
Makes sense, documented that in "Canary releases" section of "Development" README main heading 👍 |
jirispilka
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.
👍🏻
This PR improves the pre-release CI/CD workflow. It now only runs when the PR has a beta tag (including when the tag is added without a new push). The calculation of the next beta release version is now more robust: it checks both NPM and Git tags for existing beta versions and uses the highest found, ensuring the next beta version is always unique and up-to-date.
closes #80
Notes: