Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update AWS/S3 config #764

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Update AWS/S3 config #764

merged 1 commit into from
Jul 25, 2024

Conversation

rowanseymour
Copy link
Member

No description provided.

Endpoint: b.config.S3Endpoint,
Region: b.config.S3Region,
DisableSSL: b.config.S3DisableSSL,
Copy link
Member Author

Choose a reason for hiding this comment

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

Best I can tell this isn't needed if you're providing an endpoint as a full URL with http or https

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.90%. Comparing base (5d72fe3) to head (aa4ca63).

Files Patch % Lines
backends/rapidpro/backend.go 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #764      +/-   ##
==========================================
+ Coverage   75.87%   75.90%   +0.02%     
==========================================
  Files         111      111              
  Lines       10392    10386       -6     
==========================================
- Hits         7885     7883       -2     
+ Misses       1788     1784       -4     
  Partials      719      719              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MaxRetries: 3,
}
if b.config.AWSAccessKeyID != "" && !b.config.AWSUseCredChain {
s3config.AWSAccessKeyID = b.config.AWSAccessKeyID
s3config.AWSSecretAccessKey = b.config.AWSSecretAccessKey
Copy link
Member Author

Choose a reason for hiding this comment

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

These are being set above anyway. The logic should be that if AWSUseCredChain is set, you get S3 storage if tho AWSAccessKeyID and AWSSecretAccessKey are blank.

@rowanseymour rowanseymour requested a review from norkans7 July 25, 2024 16:34
@@ -655,7 +649,7 @@ func (b *backend) SaveAttachment(ctx context.Context, ch courier.Channel, conten

orgID := ch.(*Channel).OrgID()

path := filepath.Join(b.config.S3AttachmentsPrefix, strconv.FormatInt(int64(orgID), 10), filename[:4], filename[4:8], filename)
path := filepath.Join("attachments", strconv.FormatInt(int64(orgID), 10), filename[:4], filename[4:8], filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep that as media?
Are the files actually moved on that path now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just what is set on the default config (used by tests) that we always overwrite on prod to attachments. RP, mailroom and courier should all use the same attachments prefix.

@rowanseymour rowanseymour merged commit 39691f2 into main Jul 25, 2024
6 of 7 checks passed
@rowanseymour rowanseymour deleted the update_config branch July 25, 2024 21:08
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants