sqs-admin icon indicating copy to clipboard operation
sqs-admin copied to clipboard

feat: credentials config

Open ilteoood opened this issue 2 years ago • 7 comments

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

ilteoood avatar Oct 30 '23 20:10 ilteoood

Ping @PacoVK

ilteoood avatar Nov 02 '23 08:11 ilteoood

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 :)

PacoVK avatar Nov 02 '23 09:11 PacoVK

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 🤔

ilteoood avatar Nov 03 '23 21:11 ilteoood

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 avatar Nov 07 '23 17:11 PacoVK

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 :)

PacoVK avatar Nov 15 '23 21:11 PacoVK

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?

ilteoood avatar Nov 15 '23 22:11 ilteoood

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

PacoVK avatar Nov 16 '23 05:11 PacoVK