Add s3_bucket_exists on storage backend #1
Conversation
| ) | ||
|
|
||
|
|
||
| def action_ensure_bucket_exists(self): |
There was a problem hiding this comment.
to clarify:
- btn
action_ensure_bucket_existsto test if the bucket exists by calling_ensure_bucket_exists
I am not sure how do we want to test bucket connection here, I assumed that we want to start flow for getting bucket and forcing flag to True so we make sure call to head_bucket is made.
There was a problem hiding this comment.
TBH I forgot I added a generic way for defining validation of backends. Please check action_test_config in storage_backend -> it works if you define a validate_config method on the adapter.
Which means that we can:
- rename
s3_bucket_existstovalidatedand move it to storage_backend and letaction_test_configset it to True or False when needed (do this in its own commit pls) - define
validate_configand make it call_check_bucket_exists - get rid of the custom btn
There was a problem hiding this comment.
@simahawk just checking both modules, in storage_backend there is already a field 'has_validation'. Are we sure we want another field with basically same functionality there?
ajaniszewska-dev
force-pushed
the
s3-improve-get-bucket_add_flag
branch
3 times, most recently
from
7bc809f to
7c06954
Compare
ajaniszewska-dev
force-pushed
the
s3-improve-get-bucket_add_flag
branch
from
7c06954 to
3e0f851
Compare
ajaniszewska-dev
force-pushed
the
s3-improve-get-bucket_add_flag
branch
from
f8d2050 to
122d400
Compare
Codecov Report
@@ Coverage Diff @@
## s3-improve-get-bucket #1 +/- ##
=========================================================
- Coverage 86.51% 86.34% -0.17%
=========================================================
Files 41 41
Lines 1305 1311 +6
=========================================================
+ Hits 1129 1132 +3
- Misses 176 179 +3
Continue to review full report at Codecov.
|
| ) | ||
| has_validation = fields.Boolean(compute="_compute_has_validation") | ||
|
|
||
| validated = fields.Boolean(string="Validated",compute="_compute_has_validation") |
There was a problem hiding this comment.
what is this? It should be a normal stored field, not computed :/
| for rec in self: | ||
| adapter = self._get_adapter() | ||
| rec.has_validation = hasattr(adapter, "validate_config") | ||
| rec.validated = rec.has_validation = hasattr(adapter, "validate_config") |
| s3 = boto3.resource("s3", **params) | ||
| bucket_name = self.collection.aws_bucket | ||
| bucket = self._get_or_create_bucket(s3, bucket_name, **params) | ||
| self.collection.validated = True |
There was a problem hiding this comment.
this writes on EVERY lookup for the bucket, pls drop it
| s3.meta.client.head_bucket(Bucket=bucket_name) | ||
| # The call above is expensive, avoid it when possible. | ||
| EXISTING_BUCKETS.append(bucket_name) | ||
| self.collection.validated = True |
There was a problem hiding this comment.
you are writing on this a second time because is done already on line 148.
Drop this and rely only on the flag that this method should return and update the flag only in validate_config
3 participants
Uh oh!
There was an error while loading. Please reload this page.