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

feat: credentials config #707

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ilteoood
Copy link

With this PR I would like to introduce credentials configurations through environment variables

@ilteoood
Copy link
Author

ilteoood commented Nov 2, 2023

Ping @PacoVK

@PacoVK
Copy link
Owner

PacoVK commented Nov 2, 2023

Thanks for your contribution! While i see this may be usefull this PR needs more than just add the ENVs.

  • The local endpoint overwrite needs to be optional, otherwise one needs to know the AWS endpoint and set the config explicitly
  • The UI needs some change, because local cloud emulators like Localstack are less strict about parameters that are send to the API (eg. FiFoQueue property is not allowed to be sent for Standard Queues (although a falsy value is accepted)

TL;DR; i will accept this feature, but it must work with the real API prior i can accept this PR :)

@ilteoood
Copy link
Author

ilteoood commented Nov 3, 2023

Hi @PacoVK,
I've made the endpoint URL optional, but I don't understand the second comment: the FifoQueue parameter seems to be used just during the queue creation, and there is legit to always have that parameter 🤔

@PacoVK
Copy link
Owner

PacoVK commented Nov 7, 2023

Thanks again! As soon as I find time I test it again. So far the parameter caused bad request even though it was set to false

@PacoVK
Copy link
Owner

PacoVK commented Nov 15, 2023

After reviewing i still see some work to do here to make it work.

  • still the UI needs some change, because local cloud emulators like Localstack are less strict about parameters that are send to the API (eg. FiFoQueue property is not allowed to be sent for Standard Queues (although a falsy value is accepted) --> you can easily verify this if you try to connect SQS-Admin with your changes and try to create a new Queue on AWS via SQS-Admin
  • Now, with the changes, the local environment needs config. SQS-Admin is primarily for local development. Hence, the whole Endpoint setting must be optional. With the current changes, the user is enforced to set SQS_ENDPOINT_URL variable. I think this must be avoided through local development first approach.

I really appreciate the changes you make. Please also test them in advance to verify the changes you made do not break anything that exists :)

@ilteoood
Copy link
Author

Initially you said

The local endpoint overwrite needs to be optional, otherwise one needs to know the AWS endpoint and set the config explicitly

Now you say:

Now, with the changes, the local environment needs config

...which is normal. If the endpoint definition is optional then the library uses the aws one by default.
Can you chose which behaviour needs to be implemented?

@PacoVK
Copy link
Owner

PacoVK commented Nov 16, 2023

Sorry if it was unclear, what i essentially meant was that the whole endpoint resolver must be optional. If i run your code, with AWS credentials set, then there is an error about the config, and it only works if the whole endpoint resolver has been removed. That said we could make the whole EP resolver optional and thus also keep the localstack url, in case there has been no SQS_ACCESS_KEY_ID set. Is that understandable? You can easily test your code by just connecting SQS Admin to a real AWS account. There are still other parts as i mentioned that need further changes in order to work

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.

None yet

2 participants