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

fix(electron-release-publisher): change api/version endpoint in PublisherERS to use versions/sorted #3431

Merged
merged 4 commits into from
Dec 1, 2023

Conversation

kgallagher52
Copy link
Contributor

@kgallagher52 kgallagher52 commented Nov 30, 2023

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Issue: #3430

Electron Release Server, specifically the Sails blueprint, only allows a maximum of 30 records to be returned on the endpoint /api/version. The problem with this is when you call:

const versions: ERSVersion[] = await (await authFetch('api/version')).json();

You get back a random 30 records. If you have 30 or more versions that have been created, this poses a problem. If you don't get the version that you are creating, say, 0.0.1_stable, on that /api/version call, it will try to create it again even though it's already been created in the database. It will result in the error:

Error: ERS publish failed with status code: 400 (*** /api/version)

If you look at the Electron Release Server error, it's giving you:

AdapterError: Would violate uniqueness constraint-- a record already exists with conflicting value(s).

Because the record has been created, but because /api/version only returns 30 records, you may not have been given that version on the call, so the check will come back that it does not exist.

Solution:

This change simply switches the GET call from /api/version to /versions/sorted, so you won't run into this issue when you have exceeded 30 versions. The endpoint I changed it to gets all versions using semvr and is documented on Electron Release Server https://github.com/ArekSredzki/electron-release-server/blob/master/docs/api.md

I also updated the type for:

const versions: ERSVersion[]

so it corresponds correctly with the return value of /versions/sorted from Electron Release Server.

Images of the errors that get thrown because of this
Screenshot 2023-11-29 at 10 09 09 PM
Screenshot 2023-11-29 at 10 09 14 PM

@kgallagher52 kgallagher52 requested a review from a team as a code owner November 30, 2023 05:10
@kgallagher52 kgallagher52 changed the title Fixing publisher-electron-release-server version check - https://github.com/electron/forge/issues/3430 fix(electron-release-publisher): change api/version endpoint in PublisherERS to use versions/sorted Nov 30, 2023
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

Code LGTM. Can you add a some tests?

@kgallagher52
Copy link
Contributor Author

@BlackHole1 , could I get another review? I have updated the spec to match the new endpoint. I'm also curious, once the change gets merged, does it auto-deploy so that I can use this in my project?

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM

@BlackHole1
Copy link
Member

I'm also curious, once the change gets merged, does it auto-deploy so that I can use this in my project?

Once the PR is merged, it will not be released immediately. However, we will push for this fix to be released as soon as possible.

@BlackHole1 BlackHole1 requested a review from a team December 1, 2023 01:27
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

The fix makes sense - thanks for the thorough description and the fix!

@kgallagher52
Copy link
Contributor Author

kgallagher52 commented Dec 1, 2023

@BlackHole1 @VerteDinde Thank you both. Do you know what version it will be on when it gets released so I can watch for it and is there a way for me to use it in the mean time? Also do you merge this PR? I can't merge it so i'm assuming that one of you merge it?

@BlackHole1 BlackHole1 merged commit c56f406 into electron:main Dec 1, 2023
8 checks passed
@BlackHole1
Copy link
Member

Do you know what version it will be on when it gets released so I can watch for it

It could be either 7.2.1 or 7.3.0, the specific version cannot be determined at the moment because we need to evaluate whether any other destructive changes or new features PRs have been merged.

is there a way for me to use it in the mean time?

Before the official release, you can use patch-package

If nothing goes wrong, we will publish within a week.

Brooooooklyn added a commit to toeverything/AFFiNE that referenced this pull request Feb 23, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@electron-forge/maker-base](https://github.com/electron/forge) | [`7.2.0` -> `7.3.0`](https://renovatebot.com/diffs/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@electron-forge%2fmaker-base/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@electron-forge%2fmaker-base/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |
| [@napi-rs/cli](https://github.com/napi-rs/napi-rs) | [`3.0.0-alpha.40` -> `3.0.0-alpha.41`](https://renovatebot.com/diffs/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@napi-rs%2fcli/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@napi-rs%2fcli/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>electron/forge (@&#8203;electron-forge/maker-base)</summary>

### [`v7.3.0`](https://github.com/electron/forge/releases/tag/v7.3.0)

[Compare Source](https://github.com/electron/forge/compare/v7.2.0...v7.3.0)

##### What's Changed

##### Features

-   feat(plugin-vite): upgrade to vite@5 by [@&#8203;caoxiemeihao](https://github.com/caoxiemeihao) in [electron/forge#3468
-   feat: allow a custom out dir from forge config by [@&#8203;lutzroeder](https://github.com/lutzroeder) in [electron/forge#3458
-   feat(template-vite): patch types by [@&#8203;caoxiemeihao](https://github.com/caoxiemeihao) in [electron/forge#3494
-   feat: adds default fuses to templates by [@&#8203;yangannyx](https://github.com/yangannyx) in [electron/forge#3480
-   feat(publisher-github): option to automatically generate release notes by [@&#8203;dsanders11](https://github.com/dsanders11) in [electron/forge#3484

##### Fixes

-   fix(electron-release-publisher): change api/version endpoint in PublisherERS to use versions/sorted by [@&#8203;kgallagher52](https://github.com/kgallagher52) in [electron/forge#3431
-   fix(core): packageJSON won't be found when programmatic usage instead of CLI by [@&#8203;ianho](https://github.com/ianho) in [electron/forge#3455
-   fix: actually depend on preceeding groups by [@&#8203;MarshallOfSound](https://github.com/MarshallOfSound) in [electron/forge#3438
-   fix: normalize windows version with build part correctly by [@&#8203;rickymohk](https://github.com/rickymohk) in [electron/forge#3461
-   fix: .vscode settings.json changes on open by [@&#8203;lutzroeder](https://github.com/lutzroeder) in [electron/forge#3460
-   fix(plugin-vite): package volume size to large by [@&#8203;caoxiemeihao](https://github.com/caoxiemeihao) in [electron/forge#3336

##### Performance

-   refactor: only run webpack once for multi-arch packages by [@&#8203;MarshallOfSound](https://github.com/MarshallOfSound) in [electron/forge#3437

##### Other Changes

-   chore: update Packager by [@&#8203;erikian](https://github.com/erikian) in [electron/forge#3419
-   chore: bump electronjs/node to 2.2.0 (main) by [@&#8203;electron-roller](https://github.com/electron-roller) in [electron/forge#3469
-   chore(plugins/electronegativity): correct some config types  by [@&#8203;Dogdriip](https://github.com/Dogdriip) in [electron/forge#3482
-   chore: use Dependabot to update GitHub Actions deps by [@&#8203;dsanders11](https://github.com/dsanders11) in [electron/forge#3487
-   chore: bump electronjs/node to 2.2.1 (main) by [@&#8203;electron-roller](https://github.com/electron-roller) in [electron/forge#3496

##### New Contributors

-   [@&#8203;kgallagher52](https://github.com/kgallagher52) made their first contribution in [electron/forge#3431
-   [@&#8203;rickymohk](https://github.com/rickymohk) made their first contribution in [electron/forge#3461
-   [@&#8203;lutzroeder](https://github.com/lutzroeder) made their first contribution in [electron/forge#3460
-   [@&#8203;ianho](https://github.com/ianho) made their first contribution in [electron/forge#3455
-   [@&#8203;yangannyx](https://github.com/yangannyx) made their first contribution in [electron/forge#3480
-   [@&#8203;Dogdriip](https://github.com/Dogdriip) made their first contribution in [electron/forge#3482

**Full Changelog**: electron/forge@v7.2.0...v7.3.0

</details>

<details>
<summary>napi-rs/napi-rs (@&#8203;napi-rs/cli)</summary>

### [`v3.0.0-alpha.41`](https://github.com/napi-rs/napi-rs/releases/tag/%40napi-rs/cli%403.0.0-alpha.41)

[Compare Source](https://github.com/napi-rs/napi-rs/compare/@napi-rs/cli@3.0.0-alpha.40...@napi-rs/cli@3.0.0-alpha.41)

##### What's Changed

-   fix(cli): fallback to wasm32 if platform is not support by [@&#8203;Brooooooklyn](https://github.com/Brooooooklyn) in [napi-rs/napi-rs#1967
-   fix(cli): allow more platform & arch fallback to wasm by [@&#8203;Brooooooklyn](https://github.com/Brooooooklyn) in [napi-rs/napi-rs#1969

**Full Changelog**: https://github.com/napi-rs/napi-rs/compare/napi@2.15.3...[@&#8203;napi-rs/cli](https://github.com/napi-rs/cli)[@&#8203;3](https://github.com/3).0.0-alpha.41

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/toeverything/AFFiNE).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMDAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIwMC4wIiwidGFyZ2V0QnJhbmNoIjoiY2FuYXJ5In0=-->
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.

3 participants