Skip to content

fix(s3): fix handling verify_bucket_exists parameter - #39017

Merged
tcitworld merged 1 commit into
masterfrom
fix/s3/verify_bucket_exists
Aug 31, 2023
Merged

fix(s3): fix handling verify_bucket_exists parameter#39017
tcitworld merged 1 commit into
masterfrom
fix/s3/verify_bucket_exists

Conversation

@tcitworld

@tcitworldtcitworld commented Jun 26, 2023

Copy link
Copy Markdown
Member

If 'verify_bucket_exists' is set to false in the config.php s3 configuration, it's supposed to avoid verifying that the bucket exists. However empty(falsy) will always return true, so this condition would not work.

Follow-up to #23932

Checklist

@tcitworldtcitworld added this to the Nextcloud 28 milestone Jun 26, 2023
@tcitworld
tcitworld requested review from a team, ArtificialOwl, icewind1991, juliusknorr and nfebe and removed request for a teamJune 26, 2023 14:45
@tcitworld

tcitworld commented Jun 26, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to stable27

@tcitworld

tcitworld commented Jun 26, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to stable26

@tcitworld

tcitworld commented Jun 26, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to stable25

joshtrichards
joshtrichards approved these changes Jul 2, 2023
View reviewed changes

@joshtrichardsjoshtrichards left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch.

Documentation covered few weeks back in nextcloud/documentation#10437 & nextcloud/documentation#10436

@joshtrichards

joshtrichards commented Jul 2, 2023

Copy link
Copy Markdown
Member

There are also (some # of) installations out there with autocreate in their config to toggle this functionality with S3 (because until recently autocreate was in our docs - though it did nothing - oops!)

Probably too late (risky) to make that parameter equivalent now without blowback. Also, probably not a big deal as long as it: (a) now works (b) the correct parameter is now documented.

@juliusknorr
juliusknorrforce-pushed the fix/s3/verify_bucket_exists branch from f208387 to 6df781eCompareJuly 10, 2023 15:45
@juliusknorrjuliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 10, 2023
@jdavid82

This comment was marked as off-topic.

Sign in to view
@jdavid82jdavid82 mentioned this pull request Jul 22, 2023
Open
@EchedelleLR

This comment was marked as off-topic.

Sign in to view
@baldorch

This comment was marked as off-topic.

Sign in to view
@jdavid82

This comment was marked as off-topic.

Sign in to view
@EchedelleLR

This comment was marked as off-topic.

Sign in to view
@baldorch

This comment was marked as off-topic.

Sign in to view
@jdavid82

This comment was marked as off-topic.

Sign in to view
@tcitworld

tcitworld commented Jul 26, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I've marked the above messages off-topic as the bug was present from the very beginning this feature (and the parameter) was introduced.
#23932

@susnux
susnuxforce-pushed the fix/s3/verify_bucket_exists branch from 6df781e to c2fc3b8CompareAugust 2, 2023 14:42
@kesselb
kesselbforce-pushed the fix/s3/verify_bucket_exists branch from c2fc3b8 to 3726f3fCompareAugust 31, 2023 13:54
@tcitworld@kesselb
fix(s3): fix handling verify_bucket_exists parameter
3f28fc5
If 'verify_bucket_exists' is set to false in the config.php s3 configuration, it's supposed to avoid
verifying that the bucket exists. However empty(falsy) will always return true, so this condition
would not work.
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@kesselb
kesselbforce-pushed the fix/s3/verify_bucket_exists branch from 3726f3f to 3f28fc5CompareAugust 31, 2023 19:36
@tcitworld
tcitworld merged commit 065e1e8 into masterAug 31, 2023
@tcitworld
tcitworld deleted the fix/s3/verify_bucket_exists branch August 31, 2023 21:10
This was referenced Aug 31, 2023
Merged
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshtrichardsjoshtrichardsjoshtrichards approved these changes
@susnuxsusnuxsusnux approved these changes
@juliusknorrjuliusknorrjuliusknorr approved these changes
@icewind1991icewind1991Awaiting requested review from icewind1991
@ArtificialOwlArtificialOwlAwaiting requested review from ArtificialOwlArtificialOwl is a code owner automatically assigned from nextcloud/server-backend
@nfebenfebeAwaiting requested review from nfebenfebe is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: external storagefeature: object storage

Projects

None yet

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

7 participants

@tcitworld@joshtrichards@jdavid82@EchedelleLR@baldorch@susnux@juliusknorr