Fix #3931: Add unversioned flag to Package.set() and Package.set_dir()

[drernie]

Rewrite of #3924

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.48%. Comparing base (7c6edd1) to head (56864f7).
Report is 16 commits behind head on master.

❗ Current head 56864f7 differs from pull request most recent head e845b36. Consider uploading reports for the commit e845b36 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3927       +/-   ##
===========================================
+ Coverage   36.58%   84.48%   +47.90%     
===========================================
  Files         719       40      -679     
  Lines       31980     3488    -28492     
  Branches     4707        0     -4707     
===========================================
- Hits        11700     2947     -8753     
+ Misses      19116      541    -18575     
+ Partials     1164        0     -1164     

Flag	Coverage Δ
api-python	?
catalog	?
lambda	84.48% <ø> (-3.26%)	⬇️

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]

First stab at trying to mock S3 calls. Any advice appreciated...

[sir-sigurd]

@drernie
WDYT about having explicit flag to use non-versioned physical keys instead of fallback?

I think it would useful in these cases:

1. ListBucketVersions is not granted by accident
2. you have ListBucketVersions permission, but don't have GetObjectVersion permission, in that case physical keys with version IDs are useless for you

[drernie]

@drernie WDYT about having explicit flag to use non-versioned physical keys instead of fallback?

As we discussed, the (good) reason to throw an error on insufficient version permissions is to ensure they realize their data is not being versioned. Of course, right now we do NOT warn them if the bucket simply lacks versions.

The two consistent options would seem to be:

1. Ignore version-permission issues the same way we ignore buckets lacking versioning
2. Add an "ignore-missing-versioning" flag which does the above when True, but fails for both when False (and has legacy behavior if missing)

Is that right?

[sir-sigurd]

there are more things we should have in mind

1. list_object_versions() returns "null" as VersionId for non-versioned buckets
2. AFAIK if you enable versioning for non-versioned bucket the existing objects still would have "null" as VersionId

[drernie]

Okay, this seems a minimal PR that addresses the most urgent use case for Omics.
Does this work for you @sir-sigurd ?

Also: is there any way we could put out a 5.4.1 pre-release next Monday, so we can announce "Omics support" at my Tuesday session (right before the AWS Omics talk)?

[sir-sigurd]

Okay, this seems a minimal PR that addresses the most urgent use case for Omics. Does this work for you @sir-sigurd ?

my idea was that we do only list_objects or only list_object_versions depending on the value of the new flag
with current code if you have ListBucketVersions you get versioned physical keys which are useless if you don't have GetObjectVersion

IMO we should also fix Package.set() in this PR as well because it's quite related (it should fix Package.set() with Omics)

Also: is there any way we could put out a 5.4.1 pre-release next Monday, so we can announce "Omics support" at my Tuesday session (right before the AWS Omics talk)?

I think yes, though it should be 5.5.0 because it's a new feature

[drernie]

Ah. So should the flag actually be "unversioned=True", so that it explicitly forces the code to ignore all versioning?

[drernie]

And should the default be None, so unversioned=False throws an error for unversioned buckets? Is there even an easy way to test for that?

[drernie]

@sir-sigurd Are we good? Should I bump the version to 5.5.0a1, or will you do that when you push the pre-release?

[sir-sigurd]

@sir-sigurd Are we good? Should I bump the version to 5.5.0a1, or will you do that when you push the pre-release?

we need a branch without breaking changes to cut 5.x release

[drernie]

Recommended:

• Review with Aneesh
• Release at 6.0.0b1

[akarve]

And should the default be None, so unversioned=False throws an error for unversioned buckets? Is there even an easy way to test for that?

Should the default for what be None?

There is a GetBucketVersioning call possible but not everyone will have permissions to it. There are in fact three possible states for the bucket versioning: Suspended, Enabled, <no status>. The latter means bucket versioning has never been on.

As for Sergey's comment from memory (but I didn't try this recently) I believe that if you turn versioning on, or on then off, objects present before or created after versioning was off indeed have a version id of null. I have very bad Internet where I am. Let me read the PR a second time and see if I can be more helpful.

[akarve]

Although we should in a separate PR distinguish between versioned and unversioned buckets in various ways throughout the product, we currently do not and have not, therefore this changeset should not have any conditionals to warn about versioned or unversioned state for reasons of consistency and backwards compatibility.

[drernie]

this changeset should not have any conditionals to warn about versioned or unversioned state

Okay, so it sounds like you are okay with this PR as it is, right? Can @sir-sigurd move forward with merging this and cutting a pre-release?

[akarve]

@sir-sigurd this is ready for pre-release

[drernie]

Although we should in a separate PR distinguish between versioned and unversioned

Created a new issue to track that. Please add additional comments there.

[sir-sigurd]

There is a GetBucketVersioning call possible but not everyone will have permissions to it.

currently we don't grant it to the stack users

There are in fact three possible states for the bucket versioning: Suspended, Enabled, <no status>. The latter means bucket versioning has never been on.

I'm not sure we could find some sensible behavior (that "throws an error for unversioned buckets") due to versioning could be enabled/disabled for existing buckets