Skip to content

feat: credentials config - #707

Open
ilteoood wants to merge 2 commits into
PacoVK:mainfrom
ilteoood:feat/credentials-configuration
Open

feat: credentials config#707
ilteoood wants to merge 2 commits into
PacoVK:mainfrom
ilteoood:feat/credentials-configuration

Conversation

@ilteoood

Copy link
Copy Markdown

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

@ilteoood

ilteoood commented Nov 2, 2023

Copy link
Copy Markdown
Author

Ping @PacoVK

@PacoVK

PacoVK commented Nov 2, 2023

Copy link
Copy Markdown
Owner

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

ilteoood commented Nov 3, 2023

Copy link
Copy Markdown
Author

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

PacoVK commented Nov 7, 2023

Copy link
Copy Markdown
Owner

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

PacoVK commented Nov 15, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

PacoVK commented Nov 16, 2023

Copy link
Copy Markdown
Owner

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.

2 participants