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

Upgrade Python dependencies #593

Merged
merged 90 commits into from
Aug 9, 2022

Conversation

mabuelhagag
Copy link
Member

@mabuelhagag mabuelhagag commented Jun 29, 2022

Changelog

@jvonau
Copy link

jvonau commented Jun 29, 2022

@mabuelhagag This is looking a pile better with the updates applied, does make build complete when run on your branch?
@holta This is where I mentioned using a git checkout in place of using pypy as the source for IIAB installs.
7104375646 from the makefile would be related to

.github.env: .github.env.gpg
	@gpg --decrypt --batch --passphrase "$(GPG_PASSPHRASE)" .github.env.gpg >.github.env

github-env: .github.env
	@docker run --rm python:3.7 python -c 'import uuid; print("SUFFIX=%s" % uuid.uuid4())' >>"$(GITHUB_ENV)"
	@sed 's/^export //' <.github.env >>"$(GITHUB_ENV)"

not sure how/where $(GPG_PASSPHRASE) comes from, hope that helps.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #593 (4890699) into master (3bf8167) will increase coverage by 1.98%.
The diff coverage is 63.96%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   76.71%   78.69%   +1.98%     
==========================================
  Files          45       52       +7     
  Lines        2766     3394     +628     
==========================================
+ Hits         2122     2671     +549     
- Misses        644      723      +79     
Impacted Files Coverage Δ
opwen_email_client/domain/email/attachment.py 0.00% <ø> (ø)
opwen_email_client/domain/email/client.py 54.34% <ø> (ø)
opwen_email_client/domain/email/sql_store.py 100.00% <ø> (ø)
opwen_email_client/domain/email/store.py 97.18% <ø> (ø)
opwen_email_client/domain/email/sync.py 90.90% <ø> (-0.07%) ⬇️
opwen_email_client/domain/email/user_store.py 97.05% <ø> (ø)
opwen_email_client/util/serialization.py 100.00% <ø> (ø)
opwen_email_client/util/wtforms.py 93.75% <ø> (ø)
opwen_email_client/webapp/forms/settings.py 42.10% <ø> (+0.77%) ⬆️
opwen_email_client/webapp/ioc.py 71.79% <ø> (-6.64%) ⬇️
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mabuelhagag
Copy link
Member Author

@mabuelhagag This is looking a pile better with the updates applied, does make build complete when run on your branch? @holta This is where I mentioned using a git checkout in place of using pypy as the source for IIAB installs. 7104375646 from the makefile would be related to

.github.env: .github.env.gpg
	@gpg --decrypt --batch --passphrase "$(GPG_PASSPHRASE)" .github.env.gpg >.github.env

github-env: .github.env
	@docker run --rm python:3.7 python -c 'import uuid; print("SUFFIX=%s" % uuid.uuid4())' >>"$(GITHUB_ENV)"
	@sed 's/^export //' <.github.env >>"$(GITHUB_ENV)"

not sure how/where $(GPG_PASSPHRASE) comes from, hope that helps.

I think this is a CI variable to decrypt the .github.env.gpg for live test. @c-w Can you provide us with more information on this?

@jvonau
Copy link

jvonau commented Jun 30, 2022

Nice, making progress

@holta
Copy link

holta commented Aug 4, 2022

@mabuelhagag do you want your code reviewed quickly by @c-w or @sbathgate or anybody else?

(If so, what are the key changes / key files that QA volunteers should be looking at?)

@mabuelhagag
Copy link
Member Author

@c-w @sbathgate

@mabuelhagag
Copy link
Member Author

Updated the PR body with changelog

@mabuelhagag
Copy link
Member Author

@c-w code formatting is reverted now.

@holta
Copy link

holta commented Aug 5, 2022

I just noticed that @mabuelhagag posted a CHANGELOG for this PR at the very top of the discussion here — to help everyone understand:

Thank you Mohamad for your 4 months of dedication, with this very intensive work of yours, getting to know a non-simple community product over a third of a year.

Most people would have given up a long time ago.
This is a testament to your character.

Such people are hard to find in today's TikTok-driven world of 10-second videos.
I guess Egypt built the pyramids[*] before any other country for a reason!

[*] Though I have to tell you, Mexico's pyramid is larger than any of Egypt's. By volume anyway (-:

@holta
Copy link

holta commented Aug 5, 2022

@c-w code formatting is reverted now.

Thanks @mabuelhagag!

And feel free to lint &/or stylize your code for readability in a later / separate PR.

😺

Copy link
Member

@c-w c-w left a comment

Choose a reason for hiding this comment

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

I don’t have a horse in this race anymore so feel free to disregard this review. Obviously a lot of work went into this, but I can’t skip the feeling that it’s too much in one go. The integration tests are essentially the only sanity check for this project so disabling them is dangerous. I’d rather see an effort to keep them green on each change and perhaps smaller more incremental updates.

"${scriptdir}/3-receive-email-for-client.sh" && wait_seconds "${TEST_STEP_DELAY}"
"${scriptdir}/4-client-downloads-emails.sh"
Copy link
Member

Choose a reason for hiding this comment

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

It’s dangerous to delete these tests for a change that should be a no-op (upgrading versions). Maybe there’s too many changes in one go bundled here and there’s an actual semantic change that sneaked in?

Copy link
Member Author

Choose a reason for hiding this comment

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

I strongly agree with you and I intend to investigate those errors at the earliest chance possible.
However, as we're focused on making the client work now, I will be skipping the server deployment at this PR.

At a later stage when I work on the server, we should review the failures and get them back on.

@mabuelhagag
Copy link
Member Author

I don’t have a horse in this race anymore so feel free to disregard this review. Obviously a lot of work went into this, but I can’t skip the feeling that it’s too much in one go. The integration tests are essentially the only sanity check for this project so disabling them is dangerous. I’d rather see an effort to keep them green on each change and perhaps smaller more incremental updates.

@c-w That's exactly what I intend to do moving forward. Changes should be more atomic.

This reverts commit f3359ea.
@mabuelhagag mabuelhagag merged commit c0dd2a9 into ascoderu:master Aug 9, 2022
@holta
Copy link

holta commented Aug 9, 2022

Congrats on Lokole 0.8.1 !
https://github.com/ascoderu/lokole/releases/tag/0.8.1

Any MVP ("Minimal Viable Postage") setup suggestions for volunteers interested in trying/testing — e.g. with https://Internet-in-a-Box.org ?

@sbathgate
Copy link
Member

Great work here, apologies for the absence, been fairly overwhelmed the last few months. I'll try to keep a more diligent eye on future PR's as they arise here. Thanks for the immense effort though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants