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

Omics fixes #3924

Closed
wants to merge 47 commits into from
Closed

Omics fixes #3924

wants to merge 47 commits into from

Conversation

drernie
Copy link
Member

@drernie drernie commented Apr 3, 2024

  1. Use 'list_objects' if cannot call 'list_object_versions'
  2. Do NOT fail if the user cannot read the workflow config folder (causes many errors)
  3. Add integration test to check Omics access (will need to be removed, as it requires sales credentials)

@drernie drernie changed the base branch from master to release-6.0.0a1 April 3, 2024 05:02
@drernie drernie changed the base branch from release-6.0.0a1 to master April 3, 2024 05:02
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.19%. Comparing base (2edbba0) to head (e32beb2).

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3924       +/-   ##
===========================================
- Coverage   36.58%   24.19%   -12.39%     
===========================================
  Files         719      684       -35     
  Lines       31980    26040     -5940     
  Branches     4707     4707               
===========================================
- Hits        11700     6301     -5399     
+ Misses      19116    18575      -541     
  Partials     1164     1164               
Flag Coverage Δ
api-python ?
catalog 10.61% <ø> (ø)
lambda 87.74% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drernie drernie requested a review from sir-sigurd April 3, 2024 05:09
@drernie
Copy link
Member Author

drernie commented Apr 3, 2024

  1. Can we add some sort of standard option to NOT always have to read the workflow config? This drove me crazy at the NextFlow summit.
  2. The integration test is horrible, in that it requires the sales profile to test against Omics. Should I just delete it, or is there a saner way to test the "versions not allowed" use case?

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.

this PR needs to be split at least into two parts: list objects logic change and workflow logic changes

api/python/quilt3/VERSION Outdated Show resolved Hide resolved
api/python/quilt3/data_transfer.py Outdated Show resolved Hide resolved
api/python/quilt3/data_transfer.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I see no reason to not having unit tests instead

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 have no idea how to test permission errors.

Copy link
Member

Choose a reason for hiding this comment

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

api/python/quilt3/workflows/__init__.py Outdated Show resolved Hide resolved
@sir-sigurd
Copy link
Member

@drernie
why don't you use master branch as base of your PR?

@drernie
Copy link
Member Author

drernie commented Apr 3, 2024 via email

@sir-sigurd
Copy link
Member

wasn’t thinking clearly when I created the branch, and now I don’t know how to change it.

you could start from scratch and force push here

objects, _ = list_object_versions(src.bucket, src_path)
try:
objects, _ = list_object_versions(src.bucket, src_path)
except botocore.exceptions.ClientError as e:
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 still too broad

Suggested change
except botocore.exceptions.ClientError as e:
except botocore.exceptions.ClientError as e:
if e.response["Error"]["Code"] != "AccessDenied":
raise

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in new PR

@drernie
Copy link
Member Author

drernie commented Apr 3, 2024

Let me just start over, rather than risk messing up git with my lack of skill:
#3927

@drernie drernie closed this Apr 3, 2024
@sir-sigurd sir-sigurd deleted the omics-fixes branch April 3, 2024 17:06
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