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.

Sign up for GitHub

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

Jump to bottom

Update CI/CD configuration #253

Merged
merged 6 commits into from
Jul 12, 2022
Merged

Update CI/CD configuration #253

merged 6 commits into from
Jul 12, 2022

Conversation

giffels
Copy link
Member

@giffels giffels commented Jul 7, 2022 •
edited
Loading

This pull request:

  • enables deployment test on Ubuntu 22.04 LTS
  • enables docker builds of Ubuntu 22.04 LTS deployment test image
  • makes unittests on pypy optional removes pypy unittest due to broken dependencies
  • Fix Flake B023 in asyncbulkcall as suggested by @maxfischer2781
  • Update NodeJs to a maintained version 18

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2022 •
edited
Loading

Codecov Report

Merging #253 (0e2c0d3) into master (9521281) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #253   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files          54       54           
  Lines        2144     2144           
=======================================
  Hits         2130     2130           
  Misses         14       14
Impacted Files Coverage Δ
tardis/utilities/asyncbulkcall.py 96.15% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9521281...0e2c0d3. Read the comment docs.

@giffels
Copy link
Member Author

giffels commented Jul 7, 2022

@maxfischer2781 could take a look at https://github.com/MatterMiners/tardis/runs/7230668002?check_suite_focus=true, please? Can we ignore this or should we take any actions?

@maxfischer2781
Copy link
Member

That's a veritable bug. It should capture the value:

task.add_done_callback(lambda _, task=task: self._bulk_tasks.discard(task))

@giffels giffels force-pushed the ci-cd-updates branch 2 times, most recently from 556ef82 to db4e89a Compare July 7, 2022 10:45
@giffels giffels marked this pull request as ready for review July 8, 2022 08:04
giffels
giffels commented Jul 8, 2022
View reviewed changes
tardis/utilities/asyncbulkcall.py
@@ -125,7 +125,7 @@ async def _bulk_dispatch(self):
# track tasks via strong references to avoid them being garbage collected.
# see bpo#44665
self._bulk_tasks.add(task)
task.add_done_callback(lambda _: self._bulk_tasks.discard(task))
task.add_done_callback(lambda _, task=task: self._bulk_tasks.discard(task))
Copy link
Member Author

@giffels giffels Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

During development the newest flake version complaint about B023 in this line. So, as agreed in the this PR, I have added a fix.

@giffels giffels requested review from maxfischer2781, a team and eileen-kuehn and removed request for a team July 8, 2022 08:08
@giffels giffels added the Improvement Code Improvements label Jul 8, 2022
@giffels
Copy link
Member Author

giffels commented Jul 8, 2022

I tried to make the pypy unittest optional, but using continue-on-error did not work out. Since we are not claiming to support pypy, I have for now removed the unittest, since it is broken due to a broken dependency.

maxfischer2781
maxfischer2781 approved these changes Jul 8, 2022
View reviewed changes
Copy link
Member

@maxfischer2781 maxfischer2781 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. I would have hoped we can keep PyPy for detecting lifetime issues, but that's not worth such dependency issues.

eileen-kuehn
eileen-kuehn approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot 👍

@giffels giffels merged commit 0704808 into MatterMiners:master Jul 12, 2022
@giffels giffels deleted the ci-cd-updates branch July 12, 2022 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxfischer2781 maxfischer2781 maxfischer2781 approved these changes

@eileen-kuehn eileen-kuehn eileen-kuehn approved these changes

Assignees
No one assigned
Labels
Improvement Code Improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giffels @codecov-commenter @maxfischer2781 @eileen-kuehn