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

PR: Check for asset availability when checking for updates #22598

Merged
merged 7 commits into from
Oct 16, 2024

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Sep 27, 2024

Description of Changes

  • Check that the download asset is available before alerting the user to an available update. If the asset is not available, also check the next-latest release, and so on, until either no update or no asset is available.
  • Use osascript in order not to pollute shell history on macOS.
  • Ensure UpdateWorker.channel is assigned.

Issue(s) Resolved

Fixes #22566
Fixes #22572

@mrclary mrclary changed the title PR: Update manager PR: Check for asset availability when checking for update Sep 27, 2024
@ccordoba12 ccordoba12 added this to the v6.0.2 milestone Sep 29, 2024
@mrclary mrclary force-pushed the update-manager branch 2 times, most recently from 379eced to b2da67c Compare October 1, 2024 19:50
@mrclary mrclary requested a review from ccordoba12 October 4, 2024 06:17
@mrclary mrclary force-pushed the update-manager branch 3 times, most recently from 68153cd to a0ec21a Compare October 10, 2024 20:20
@mrclary mrclary force-pushed the update-manager branch 2 times, most recently from b558ba3 to 6552f65 Compare October 14, 2024 21:37
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Initial review for you @mrclary.

spyder/plugins/updatemanager/tests/test_update_manager.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/widgets/update.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/tests/test_update_manager.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/tests/test_update_manager.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/tests/test_update_manager.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Oct 15, 2024

Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 235:80: E501 line too long (83 > 79 characters)

Comment last updated at 2024-10-16 16:39:19 UTC

@mrclary
Copy link
Contributor Author

mrclary commented Oct 15, 2024

Initial review for you @mrclary.

@ccordoba12, thank you for suggesting the TypedDict 👍🏼

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Second round of suggestions for you @mrclary, then this should be ready (you can add them to the commit where you addressed my initial review, if you want).

spyder/plugins/updatemanager/workers.py Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
…o an available update.

Create a function that determines the asset name, update type, and url, depending on the latest release and the current Spyder version.
Fixes spyder-ide#22566
…WorkerDownloadInstaller.latest_release are now packaging.version.Version objects instead of strings.

We can now use packaging.version mechanisms for comparison, sorting, and stable release checks.
Added test_update_no_asset and test_get_asset_info
@mrclary mrclary force-pushed the update-manager branch 2 times, most recently from 31b13da to c171a8c Compare October 15, 2024 18:47
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Last nitpick @mrclary.

spyder/plugins/updatemanager/workers.py Outdated Show resolved Hide resolved
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @mrclary!

@ccordoba12 ccordoba12 changed the title PR: Check for asset availability when checking for update PR: Check for asset availability when checking for updates Oct 16, 2024
@ccordoba12 ccordoba12 merged commit f9dfab7 into spyder-ide:master Oct 16, 2024
17 checks passed
@ccordoba12
Copy link
Member

@meeseeksdev please backport to 6.x

Copy link

lumberbot-app bot commented Oct 16, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 f9dfab7ac825489a86103cfce8c8d80788daa413
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #22598: PR: Check for asset availability when checking for updates'
  1. Push to a named branch:
git push YOURFORK 6.x:auto-backport-of-pr-22598-on-6.x
  1. Create a PR against branch 6.x, I would have named this PR:

"Backport PR #22598 on branch 6.x (PR: Check for asset availability when checking for updates)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

ccordoba12 added a commit to ccordoba12/spyder that referenced this pull request Oct 17, 2024
@mrclary mrclary deleted the update-manager branch October 17, 2024 01:50
ccordoba12 added a commit that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants