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

Test quilt3 on Win on all Python versions #2900

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Conversation

eode
Copy link
Contributor

@eode eode commented Jun 15, 2022

Description

Cleaner redo of #2861 after incorrect rebase.

TODO

  • [n/a] Unit tests
  • [n/a] Automated tests (e.g. Preflight)
  • [n/a] Documentation
    • [n/a] Python: Run build.py for new docstrings
    • [n/a] JavaScript: basic explanation and screenshot of new features
  • Changelog entry

@eode eode requested a review from sir-sigurd June 15, 2022 21:16
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #2900 (e3bf6fb) into master (34520fc) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2900   +/-   ##
=======================================
  Coverage   39.95%   39.95%           
=======================================
  Files         593      593           
  Lines       26870    26870           
  Branches     3889     3889           
=======================================
  Hits        10736    10736           
  Misses      15015    15015           
  Partials     1119     1119           
Flag Coverage Δ
api-python 90.71% <100.00%> (ø)
catalog 12.61% <ø> (ø)
lambda 87.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/python/quilt3/packages.py 92.89% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@sir-sigurd sir-sigurd left a comment

Choose a reason for hiding this comment

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

It looks like a real bug fix so changelog entry is needed.

@@ -1462,7 +1462,7 @@ def check_latest_hash():
def physical_key_is_temp_file(pk):
if not pk.is_local():
return False
return pathlib.Path(pk.path).parent == APP_DIR_TEMPFILE_DIR
return pathlib.Path(pk.path).parent.resolve() == APP_DIR_TEMPFILE_DIR.resolve()
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to do this normalization when we create PhysicalKey instances?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe but I'd suggest a separate PR as there's more risk in changing the PhysicalKey interface, which isn't broken so we should be careful fixing it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree with both of those points. In theory, the resolution should not have a problematic effect -- but knowing definitively is another matter.

PR #2900, Test Quilt3 on Win on all Python versions
@eode eode requested review from sir-sigurd and akarve June 17, 2022 15:25
@eode eode merged commit 3babfd1 into master Jun 17, 2022
@eode eode deleted the test-win-all-pythons-rebase branch June 17, 2022 15:35
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