Skip to content

Follow up/11117 fixes and testing#13607

Merged
opadron merged 14 commits intospack:developfrom
opadron:follow-up/11117-fixes-and-testing
Dec 9, 2019
Merged

Follow up/11117 fixes and testing#13607
opadron merged 14 commits intospack:developfrom
opadron:follow-up/11117-fixes-and-testing

Conversation

@opadron
Copy link
Copy Markdown
Member

@opadron opadron commented Nov 5, 2019

Follows up on #11117.

  • incorporate additional feedback from S3 upload and download #11117 that was not merged.
  • fix an issue preventing prefix queries on AWS-hosted S3 buckets
  • adds more tests

@opadron opadron requested a review from tgamblin November 5, 2019 22:39
opadron
opadron commented Nov 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member Author

@opadron opadron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tgamblin FYI. See comment describing what I believe is an issue with SpackCommands that is preventing the tests from completing.

@opadron opadron force-pushed the follow-up/11117-fixes-and-testing branch from 972fb7f to 0cef46f Compare November 14, 2019 18:33
wylderkeane
wylderkeane reviewed Nov 19, 2019
View reviewed changes
@scottwittenburg
Copy link
Copy Markdown
Contributor

scottwittenburg commented Dec 2, 2019

@opadron Do you think this PR should fix the problem I'm experiencing in pipelines at the moment?

>>> import spack.binary_distribution as bindist
>>> bindist.generate_package_index("s3://spack-public/mirror/build_cache")
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/work/spack/lib/spack/spack/binary_distribution.py", line 281, in generate_package_index
    for entry in web_util.list_url(cache_prefix)
  File "/work/spack/lib/spack/spack/util/web.py", line 348, in list_url
    for key in _iter_s3_prefix(s3, url)))
  File "/work/spack/lib/spack/spack/util/web.py", line 347, in <genexpr>
    key.split('/', 1)[0]
  File "/work/spack/lib/spack/spack/util/web.py", line 328, in _iter_s3_prefix
    client, url, num_entries, start_after=key)
  File "/work/spack/lib/spack/spack/util/web.py", line 317, in _list_s3_objects
    for entry in result['Contents']
KeyError: 'Contents'

@scottwittenburg
Copy link
Copy Markdown
Contributor

scottwittenburg commented Dec 2, 2019

@opadron I just tested it out, and it seems like it does fix that problem, so nevermind. But are you still trying to get that patch coverage up? Or could this be reviewed and merged?

@opadron
Copy link
Copy Markdown
Member Author

opadron commented Dec 3, 2019

This PR is ready for review and merge. I don't see any uncovered parts of the patch that can be covered without extensive S3 mocking.

tgamblin
tgamblin requested changes Dec 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

@opadron: this is great it covers a lot of code from #11117! I have some minor refactor/change requests -- see below.

@scottwittenburg: I looked at the diff; the patch coverage is small because the PR is nearly all tests and only a little code. The total project number looks great to me for this PR.

@opadron
Copy link
Copy Markdown
Member Author

opadron commented Dec 9, 2019

@tgamblin ready

@opadron opadron merged commit 0592c58 into spack:develop Dec 9, 2019
@opadron opadron deleted the follow-up/11117-fixes-and-testing branch December 9, 2019 22:23
@opadron
Copy link
Copy Markdown
Member Author

opadron commented Dec 9, 2019

🎉

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Dec 9, 2019

@opadron: I was writing a commit message for this. There is no need to merge it if I approve it.

@opadron
Copy link
Copy Markdown
Member Author

opadron commented Dec 10, 2019

I see. My apologies. I'll hold off on merging for PRs that you approve.

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

Reviewers

@tgamblin tgamblin tgamblin approved these changes

+1 more reviewer

@wylderkeane wylderkeane wylderkeane left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fetching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@opadron @scottwittenburg @tgamblin @wylderkeane @adamjstewart