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

sources: Allow configuring a custom AWS region #448

Merged
merged 7 commits into from
Sep 10, 2021

Conversation

brancz
Copy link
Contributor

@brancz brancz commented May 31, 2021

This can be useful when using an s3 compatible service, such as minio or
Ceph.

This takes a slightly simpler approach than initially suggested in #447, since rusoto already supports parsing custom regions from tuples, I decided not to introduce yet-another-representation.

Tested with this configuration:

sources:
- id: minio
  type: s3
  bucket: symbolicator
  region: ['minio', 'http://minio.minio.svc.cluster.local:9000']
  access_key: minio
  secret_key: minio123
  layout: { type: "unified" }
  is_public: true

Closes #447

@jan-auer

@brancz brancz requested a review from a team May 31, 2021 12:02
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, unless @jan-auer has objections?

@flub
Copy link
Contributor

flub commented May 31, 2021

Any chance we could add some unit tests for these various functionalities?

@brancz
Copy link
Contributor Author

brancz commented May 31, 2021

I'm afraid that would exceed my rust abilities, I'm somewhat surprised that I managed to get this to work 😅

(I'm probably missing them entirely, I think I would have been able to add a single additional test case, but I can't find tests for deserializing at all, so creating entirely new tests probably exceeds my current rust abilities)

@flub
Copy link
Contributor

flub commented May 31, 2021

heh, fair enough. Sad we don't have an existing test, I'm just a bit concerned that changes like this could easily break existing valid configs and we should have some way of avoiding this.

Could you maybe update https://github.com/getsentry/symbolicator/blob/master/docs/api/index.md#amazon-s3-bucket to represent the current state though. People shouldn't have to read the source to figure this out.

@flub
Copy link
Contributor

flub commented May 31, 2021

Also, adding a changelog entry in the same style as existing ones will make the CI happier too :)

This can be useful when using an s3 compatible service, such as minio or
Ceph.
@brancz
Copy link
Contributor Author

brancz commented Jun 1, 2021

Updated documentation and added a changelog entry.

This test should work, but shows the existing region selection is
broken currently.
@flub
Copy link
Contributor

flub commented Jun 1, 2021

I took the liberty of pushing a simple test, hope you don't mind.

It might be simple but sadly it already shows that your change breaks the previous functionality, so all existing configurations will break. Could investigate this?

@brancz
Copy link
Contributor Author

brancz commented Jun 1, 2021

Thank you! No problem at all, this will help me investigate alternative solutions such as the initially proposed string-or-struct type of construct.

@brancz
Copy link
Contributor Author

brancz commented Jun 1, 2021

Ok, I tried, but it seems that this is out of my breadth 😅 ... does anyone think they might be able to take this over the finish line?

@relaxolotl
Copy link
Contributor

I've pushed a few changes that should make this change compatible with old configs in addition to supporting custom AWS regions; @flub @Swatinem could I trouble the both of you to take a look at the changes? They're a little bit more involved now but it's honestly just a heavily modified version of https://serde.rs/string-or-struct.html.

@brancz sorry it's taken so long to get to this. Hopefully this is the last push needed to get support for this into symbolicator proper 🤞

docs/api/index.md Outdated Show resolved Hide resolved
@flub
Copy link
Contributor

flub commented Sep 10, 2021

nice! 👍

@relaxolotl relaxolotl enabled auto-merge (squash) September 10, 2021 16:04
@relaxolotl relaxolotl merged commit 6fd6e55 into getsentry:master Sep 10, 2021
@brancz brancz deleted the custom-region branch September 28, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow custom AWS region
4 participants