Add s3_bucket_exists on storage backend #1
Conversation
ajaniszewska-dev
commented
May 30, 2022
•
ajaniszewska-dev
commented
| @@ -84,3 +85,8 @@ def _selection_aws_region(self): | |||
| + AWS_REGIONS | |||
| + [("other", "Empty or Other (Manually specify below)")] | |||
| ) | |||
|
|
|||
|
|
|||
| 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.
|
| @@ -68,11 +68,12 @@ class StorageBackend(models.Model): | |||
| help="Relative path to the directory to store the file" | |||
| ) | |||
| 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 :/
| def _compute_has_validation(self): | ||
| for rec in self: | ||
| adapter = self._get_adapter() | ||
| rec.has_validation = hasattr(adapter, "validate_config") | ||
| rec.validated = rec.has_validation = hasattr(adapter, "validate_config") |
| @@ -50,6 +46,7 @@ def _get_bucket(self): | |||
| 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
| try: | ||
| 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
