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

Deps: Bump watchdog #328

Merged
merged 2 commits into from
Feb 20, 2023
Merged

Deps: Bump watchdog #328

merged 2 commits into from
Feb 20, 2023

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Nov 29, 2022

Watchdog dependency seems to be used for live monitoring of AiiDAlab app installation process (I didn't look too closely what it actually does though). The old version prevents upgrade to Python 3.10.

I checked the watchdog Changelog and it seems this should be a seamless update. I have manually tested installation from the Appstore on this branch and it seems to work. Would be good if somebody else with more experience with Appstore tested as well. You can use this image: ghcr.io/aiidalab/full-stack:pr-337 Note that this image is build with Python 3.10, so you might as well test other things. :-)

The old version prevents upgrade to Python 3.10
I checked the watchdog changelog and it seems
this should be a seamless update.
@danielhollas danielhollas marked this pull request as ready for review December 1, 2022 12:04
@danielhollas danielhollas self-assigned this Dec 2, 2022
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

The bump looks good to me. But I have no experience with this section of code. It also has no tests related. Could be good to run an installation to test it.

@danielhollas
Copy link
Contributor Author

@yakutovicha could you give this a test? This is the only thing that's keeping us from migrating to Python 3.10, so I think we should go for it.

Note that for testing this, if you want to download the image with Python 3.10 from ghcr.io, you will probably need to get a Github access token, that you then pass to Docker to authenticate. Alternatively, you can set the packages as public, I don't really see a downside to that (except possibly confusing people, who generally should always download from Dockerhub).

@danielhollas
Copy link
Contributor Author

danielhollas commented Jan 9, 2023

Probably should be tested and merged after #346, although these PRs are unrelated.

@danielhollas
Copy link
Contributor Author

@unkcpz I have merged the isAlive PR here in case you want to test it together with the Watchdog update.
I've also rebuild the image that is using this branch, so you can just do

docker pull ghcr.io/aiidalab/full-stack:pr-337

Images on ghcr.io are not public, so if you've never pulled from it, you need to first create a Github token for docker authentication, details described here: https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry#authenticating-to-the-container-registry

@unkcpz
Copy link
Member

unkcpz commented Jan 11, 2023

Thanks @danielhollas, I'll give it a test.
I did pull the full-stack from ghcr.io once. Is the image will build every time the PR in aiidalab-docker-stack updated? You use this branch specificly in pr-337? (nevermind, I find how you did that.)

@danielhollas
Copy link
Contributor Author

Yes, the image is build in the CI automatically for each commit on that PR.

@yakutovicha
Copy link
Member

Since @unkcpz is on it, I will remove myself from the reviewers. Please proceed without me here 👍

@yakutovicha yakutovicha removed their request for review January 11, 2023 22:07
@danielhollas danielhollas removed their assignment Feb 1, 2023
@unkcpz
Copy link
Member

unkcpz commented Feb 6, 2023

I give it a test and the issue report in #340 did not show. However, the https://aiidalab.github.io is down and the app-registry is broken. I 'll give it another test after @superstar54 fix it.

@unkcpz unkcpz merged commit 3e8b431 into main Feb 20, 2023
@unkcpz unkcpz deleted the update-watchdog branch February 20, 2023 08:55
@unkcpz
Copy link
Member

unkcpz commented Feb 20, 2023

I gave it a test again and all works fine. No isAlive error popped. This PR is better to go with the new release which will include #347 so I merged.

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.

None yet

3 participants