Added ability to pin the neoboard-sdk commit - #121
Conversation
|
|
which alternatives have you considered and why are they not viable? it seems unintuitive to me to use a "pull" model rather than "push" model. why isn't the CI job to update the react sdk version used on main not part of the matrix-neoboard repo instead? |
The alternative are
You mean the builds and commits from neoboard should somehow influence which sdk version has to be used in the standalone project instead of it trying to use HEAD until we get to a release PR (in that case the pinned version is used from the latest feature merge to main)? |
|
Note: with the provided CI workflow if we want to release with a specific SDK commit in mind (not the calculated HEAD of the sdk repo used in the latest feature PR merge) we can always push a commit into the "Verson Package" PR with the sdk.version. |
…file and updated to the latest commit by the github actions Signed-off-by: segnord <sergey.vasiluk@nordeck.net>
…version pinning scenarios, also a guard condition added to pin the neoboard sdk version to the file in case the sdk has been updated since the last standalone feature merge.
Signed-off-by: Milton Moura <miltonmoura@gmail.com>
Signed-off-by: Milton Moura <miltonmoura@gmail.com>
5986999 to
5e4fe44
Compare
There was a problem hiding this comment.
It looks fine to use a neoboard react sdk commit hash in the file and use it.
However I think how it is done currently is a way more complicated then it should/could be. There are also issues that could happen and there are comments to that.
In my opinion we could simply use sdk.version as is always and update it in any PR when needed and don't rely on this implicit latest commit fetching from sdk at all.
Edit: typo fixed
| run: | | ||
| RESOLVED_SHA=$(tr -d '[:space:]' < sdk.version) | ||
|
|
||
| if [[ -n "$NEOBOARD_REF" ]]; then |
There was a problem hiding this comment.
Do we need NEOBOARD_REF at all? It was added to this PR. Why not just to update sdk.version in a PR itself?
There was a problem hiding this comment.
This parameter existed before this PR, it was empty so it just checkout HEAD always, but you could enter your value.
| # A release PR merge has no pending changesets and must build from | ||
| # the pin committed in that release commit. Feature merges track | ||
| # the latest SDK and persist the resolved SHA in the release PR. | ||
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) |
There was a problem hiding this comment.
We can also merge a PR without changeset to main with no changesets, it will not be a release PR.
There was a problem hiding this comment.
This usecase is covered in the "Renovate bot" section. Merges without a changeset won't create a new release.
| # the latest SDK and persist the resolved SHA in the release PR. | ||
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) | ||
| if [[ "$PENDING_CHANGESETS" -gt 0 ]]; then | ||
| RESOLVED_SHA=$(git ls-remote https://github.com/nordeck/matrix-neoboard.git refs/heads/main | cut -f1) |
There was a problem hiding this comment.
This is really strange I would say that we basically ignore the sdk.version and just take a latest from sdk. What is the whole purpose of sdk.version then?
A merge commit will use latest from sdk, not from sdk.version.
Just think about it: use a sdk.version to develop a feature in pull request with a particular sdk.version and use another version when merged to main. Or rerun a failed job for some reason of merge commit and use another latest sdk commit with a new run.
I think one of the main purposes of this PR is to make deterministic builds.
There was a problem hiding this comment.
Release only happens when there are 0 changeset files and the project.json version is bumped. This is managed by that special PR the changeset bot maintains, sdk.version file is used then. Other times it's just being updated in the release PR automatically to follow the HEAD.
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) | ||
| if [[ "$PENDING_CHANGESETS" -gt 0 ]]; then | ||
| RESOLVED_SHA=$(git ls-remote https://github.com/nordeck/matrix-neoboard.git refs/heads/main | cut -f1) | ||
| echo "Pending changesets found: using matrix-neoboard main at $RESOLVED_SHA." |
There was a problem hiding this comment.
It also also updates "Version Packages" with latest sdk version, not with sdk.version, so a release will happen with something you may not try before. It feels like you would use a dependency in your PR and now suddently it gets bumped in release.
Or CI could suddenly just fail in "Version Packages".
Additionally "Version Packages" are forced pushed by changesets, so if something is changed in "Version Packages" manually like sdk.version it gets overridden with next PR with changeset merged.
There was a problem hiding this comment.
Yes, that's the idea, the sdk.version is getting updates on every merge to main. But when you merge the "Version Packages" to main, it's when it won't use HEAD, but the version from the file and a release will trigger.
| # This action creates/updates the "Version Packages" pull request. | ||
| # It has 2 outcomes depending if the workspace contains any changeset files and package.json version. | ||
| # changeset files exist (version): Creates/updates the "Version packages" pull request. | ||
| # Bumps the package.json version. | ||
| # Writes the SDK used by this build to sdk.version. | ||
| # no changeset files (publish): tags the current commit with the new semver. |
There was a problem hiding this comment.
When I read it if feels confusing, things are mixed up together: "Version Packages", "2 outcomes", "any", "and".
It makes to to feel like you have to check what it does and there are just few command, maybe could improve.
There was a problem hiding this comment.
It's just the changesets github action has version and publish outcomes depending on the kind of PR is merged, that may not be obvious at a glance.
| @@ -0,0 +1 @@ | |||
| 67758ec8c804e5a2aa3be5b6e0810e45b51dabc8 | |||
There was a problem hiding this comment.
I feels fine to store it in file. But I think file should be better named, sdk is very general. Something with neoboard maybe up to matrix-neoboard-react-sdk.
There was a problem hiding this comment.
There's a sdk_version.sh file so the naming followed it.
| "depcheck": "depcheck --ignores=@vitest/coverage-v8,@changesets/cli,@swc/core,@testing-library/dom,@testing-library/jest-dom,@testing-library/react,@testing-library/react-hooks,@typescript-eslint/eslint-plugin,@typescript-eslint/parser,prettier-plugin-organize-imports,identity-obj-proxy", | ||
| "deduplicate": "yarn-deduplicate", | ||
| "translate": "i18next-cli extract --sync-primary", | ||
| "changeset:version": "yarn changeset version && node scripts/update-sdk-version.mjs", |
There was a problem hiding this comment.
Do we need changeset:version in package.json, people should not use it, ci.yaml should be possible to be updated to use command directly. Maybe we don't need this at all.
There was a problem hiding this comment.
Initially the PR didn't have it and it just wrote the SHA to file. The new script contains a SHA structure verification. @mgcm may explain better.
| env: | ||
| NEOBOARD_REF: ${{ github.event.inputs.neoboard_ref }} | ||
| run: | | ||
| RESOLVED_SHA=$(tr -d '[:space:]' < sdk.version) |
There was a problem hiding this comment.
Let's consider to use sdk.version always and don't try to magically replace/refresh it's value to latest in sdk. I think this could remove all the confusion and get things easy to understand. Anyone can just update sdk.version in any PR.
There was a problem hiding this comment.
The idea was to pin the commit only during release and never think about manually tracking the HEAD of the SDK repo during feature commits to main (same workflow as befofore this PR).
| The pinned commit lives in [`sdk.version`](../sdk.version) at the repo root. | ||
| Whether it gets refreshed before a build - and what ends up tagged - depends on the trigger. |
There was a problem hiding this comment.
I think the whole document actually shows how confusingly sdk.version is currently been used.
There was a problem hiding this comment.
The release process isn't trivial, the scripts try keep up with sdk main branch automatically while freezing the version predictably for a release with frozen commit hashes.
| env: | ||
| NEOBOARD_REF: ${{ github.event.inputs.neoboard_ref }} | ||
| run: | | ||
| RESOLVED_SHA=$(tr -d '[:space:]' < sdk.version) |
There was a problem hiding this comment.
The idea was to pin the commit only during release and never think about manually tracking the HEAD of the SDK repo during feature commits to main (same workflow as befofore this PR).
| run: | | ||
| RESOLVED_SHA=$(tr -d '[:space:]' < sdk.version) | ||
|
|
||
| if [[ -n "$NEOBOARD_REF" ]]; then |
There was a problem hiding this comment.
This parameter existed before this PR, it was empty so it just checkout HEAD always, but you could enter your value.
| # A release PR merge has no pending changesets and must build from | ||
| # the pin committed in that release commit. Feature merges track | ||
| # the latest SDK and persist the resolved SHA in the release PR. | ||
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) |
There was a problem hiding this comment.
This usecase is covered in the "Renovate bot" section. Merges without a changeset won't create a new release.
| # the latest SDK and persist the resolved SHA in the release PR. | ||
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) | ||
| if [[ "$PENDING_CHANGESETS" -gt 0 ]]; then | ||
| RESOLVED_SHA=$(git ls-remote https://github.com/nordeck/matrix-neoboard.git refs/heads/main | cut -f1) |
There was a problem hiding this comment.
Release only happens when there are 0 changeset files and the project.json version is bumped. This is managed by that special PR the changeset bot maintains, sdk.version file is used then. Other times it's just being updated in the release PR automatically to follow the HEAD.
| PENDING_CHANGESETS=$(find .changeset -maxdepth 1 -name '*.md' ! -name 'README.md' | wc -l) | ||
| if [[ "$PENDING_CHANGESETS" -gt 0 ]]; then | ||
| RESOLVED_SHA=$(git ls-remote https://github.com/nordeck/matrix-neoboard.git refs/heads/main | cut -f1) | ||
| echo "Pending changesets found: using matrix-neoboard main at $RESOLVED_SHA." |
There was a problem hiding this comment.
Yes, that's the idea, the sdk.version is getting updates on every merge to main. But when you merge the "Version Packages" to main, it's when it won't use HEAD, but the version from the file and a release will trigger.
| # This action creates/updates the "Version Packages" pull request. | ||
| # It has 2 outcomes depending if the workspace contains any changeset files and package.json version. | ||
| # changeset files exist (version): Creates/updates the "Version packages" pull request. | ||
| # Bumps the package.json version. | ||
| # Writes the SDK used by this build to sdk.version. | ||
| # no changeset files (publish): tags the current commit with the new semver. |
There was a problem hiding this comment.
It's just the changesets github action has version and publish outcomes depending on the kind of PR is merged, that may not be obvious at a glance.
| The pinned commit lives in [`sdk.version`](../sdk.version) at the repo root. | ||
| Whether it gets refreshed before a build - and what ends up tagged - depends on the trigger. |
There was a problem hiding this comment.
The release process isn't trivial, the scripts try keep up with sdk main branch automatically while freezing the version predictably for a release with frozen commit hashes.
| "depcheck": "depcheck --ignores=@vitest/coverage-v8,@changesets/cli,@swc/core,@testing-library/dom,@testing-library/jest-dom,@testing-library/react,@testing-library/react-hooks,@typescript-eslint/eslint-plugin,@typescript-eslint/parser,prettier-plugin-organize-imports,identity-obj-proxy", | ||
| "deduplicate": "yarn-deduplicate", | ||
| "translate": "i18next-cli extract --sync-primary", | ||
| "changeset:version": "yarn changeset version && node scripts/update-sdk-version.mjs", |
There was a problem hiding this comment.
Initially the PR didn't have it and it just wrote the SHA to file. The new script contains a SHA structure verification. @mgcm may explain better.
| @@ -0,0 +1 @@ | |||
| 67758ec8c804e5a2aa3be5b6e0810e45b51dabc8 | |||
There was a problem hiding this comment.
There's a sdk_version.sh file so the naming followed it.
# Conflicts: # .github/workflows/publish-release.yml
|
Conflicts resolved by merging main |
✔️ Checklist
Signed-off-byline in the message (more info).We will use a "sdk.version" file to know which neoboard-sdk commit to pull in order to build the standalone version, instead of relying in the HEAD commit of the sdk.