feat: Add support for native GitHub media attachments #208
Conversation
|
Just found out one thing that attachments copy-pasted from local files are "encoded" as HTML |
suzyng83209
left a comment
suzyng83209
left a comment
There was a problem hiding this comment.
Sorry for missing this pull request for a while. All of the code looks good. I do think you need to fix some types but the test failures look transient so a re-run may resolve it.
Let me know when you want to deploy this change!
|
@suzyng83209 Thanks for the review! There is one thing still missing in the PR, and it's the update of attachments when a Github description/comment is updated. It's really important since the flow with Upd: Thought a little more about the "simpler" approach and I don't think it will work that good, so will try to implement a dynamodb thing instead. |
| def get_dynamodb_resource(): | ||
| """Get the DynamoDB resource, creating it lazily on first access.""" | ||
| global _dynamodb_resource | ||
| if _dynamodb_resource is None: | ||
| _dynamodb_resource = boto3.resource("dynamodb", region_name=AWS_REGION) | ||
| return _dynamodb_resource | ||
|
|
||
|
|
||
| def get_dynamodb_lock_client(): | ||
| """Get the DynamoDB lock client, creating it lazily on first access.""" | ||
| global _dynamodb_lock_client | ||
| if _dynamodb_lock_client is None: | ||
| _dynamodb_lock_client = DynamoDBLockClient( | ||
| get_dynamodb_resource(), | ||
| table_name=LOCK_TABLE, | ||
| lease_duration=timedelta(seconds=20), | ||
| expiry_period=timedelta( | ||
| minutes=2 | ||
| ), # The Lambda function has a 120 second timeout by default | ||
| ) | ||
| return _dynamodb_lock_client | ||
|
|
||
|
|
||
| # Create a lazy-loading module-level variable | ||
| class _LazyLockClient: | ||
| def __getattr__(self, name): | ||
| return getattr(get_dynamodb_lock_client(), name) | ||
|
|
||
|
|
||
| dynamodb_lock_client = _LazyLockClient() |
There was a problem hiding this comment.
This was suggested by cursor, and I decided to leave it. From my understanding it is a best-practice to initialize things like that lazily, so it's a small improvement to current implementation.
Let me know if you think it's better to remove it.
There was a problem hiding this comment.
Makes sense and looks good to keep - though I don't think it'll be huge improvement since we always need to lock client to be fully set up whenever we access it.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| def get_dynamodb_resource(): | ||
| """Get the DynamoDB resource, creating it lazily on first access.""" | ||
| global _dynamodb_resource | ||
| if _dynamodb_resource is None: | ||
| _dynamodb_resource = boto3.resource("dynamodb", region_name=AWS_REGION) | ||
| return _dynamodb_resource | ||
|
|
||
|
|
||
| def get_dynamodb_lock_client(): | ||
| """Get the DynamoDB lock client, creating it lazily on first access.""" | ||
| global _dynamodb_lock_client | ||
| if _dynamodb_lock_client is None: | ||
| _dynamodb_lock_client = DynamoDBLockClient( | ||
| get_dynamodb_resource(), | ||
| table_name=LOCK_TABLE, | ||
| lease_duration=timedelta(seconds=20), | ||
| expiry_period=timedelta( | ||
| minutes=2 | ||
| ), # The Lambda function has a 120 second timeout by default | ||
| ) | ||
| return _dynamodb_lock_client | ||
|
|
||
|
|
||
| # Create a lazy-loading module-level variable | ||
| class _LazyLockClient: | ||
| def __getattr__(self, name): | ||
| return getattr(get_dynamodb_lock_client(), name) | ||
|
|
||
|
|
||
| dynamodb_lock_client = _LazyLockClient() |
There was a problem hiding this comment.
Makes sense and looks good to keep - though I don't think it'll be huge improvement since we always need to lock client to be fully set up whenever we access it.
test/impl/mock_dynamodb_test_case.py
Outdated
| os.environ.setdefault("AWS_ACCESS_KEY_ID", "foobar_key") | ||
| os.environ.setdefault("AWS_SECRET_ACCESS_KEY", "foobar_secret") | ||
| # Set ENV to test so the config module provides proper test defaults | ||
| os.environ.setdefault("ENV", "test") |
There was a problem hiding this comment.
are we using this env var anywhere?
There was a problem hiding this comment.
Good catch, no, forgot to remove these changes after figuring out how to fix CI errors. This file shouldn't have any changes at all, fixed that.
Description
This PR introduces support for adding github attachments to corresponding asana task. Currently, it only happens on PR creation, will be fixed for PR updates as well soon.
Test
Added unit tests are passing together with
extract_attachmentsfunction tested on real codez PR and video/images attachments.Pull Request synchronized with Asana task
Pull Request: #208