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

Allow multiple azure instances #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noaccOS
Copy link
Contributor

@noaccOS noaccOS commented Apr 12, 2024

Allow passing connection parameters together with the container to blob functions.

Values from the parameters are prioritized over default configuration.

Fixes #44

@crertel-getthru
Copy link

This is super neat! @jakobht can you run the workflow?

Copy link
Owner

@jakobht jakobht left a comment

Choose a reason for hiding this comment

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

Looks good, I think it would be nice to add something in the Readme on this - and extend the (sadly few) examples we have.

Just to make clear what can be passed as connection_params

lib/azurex/blob.ex Show resolved Hide resolved
lib/azurex/blob/config.ex Show resolved Hide resolved
@jakobht
Copy link
Owner

jakobht commented Apr 23, 2024

Hi, added a few comments, but generally looks good! The CI failed, the linter is sad.

I just updated the build pipelines in master to check 1.16 (but drop 1.11), so please merge.

Thanks a lot for the PR!

@noaccOS noaccOS force-pushed the feat/multiple-azure-instances branch from e8a643c to 34fa38d Compare April 23, 2024 09:52
@noaccOS
Copy link
Contributor Author

noaccOS commented Apr 23, 2024

Rebased and added some more examples. I've also mentioned this feature briefly in the README.

The linter is sad because of unused optional parameters but I haven't touched that section myself.
Maybe I could remove those optional parameters for now? It's always possible to add them back later if we need them as it's a private function anyways

@noaccOS noaccOS requested a review from jakobht April 23, 2024 10:33
{:ok, HTTPoison.Response.t()} | {:error, term()}
def copy_blob(source_name, destination_name, container \\ nil) do
def copy_blob(source_name, destination_name, overrides \\ []) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest documenting that the overrides apply to both source and destination, so you can't use this method to copy between accounts.

Copy link
Owner

Choose a reason for hiding this comment

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

Not super important IMO, but documentation is always good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a line in the doc mentioning it

@jakobht
Copy link
Owner

jakobht commented Apr 30, 2024

Rebased and added some more examples. I've also mentioned this feature briefly in the README.

The linter is sad because of unused optional parameters but I haven't touched that section myself. Maybe I could remove those optional parameters for now? It's always possible to add them back later if we need them as it's a private function anyways

It should be fine to delete the unused args.

Sorry I'm slow at answering :)

@stilist
Copy link
Contributor

stilist commented Apr 30, 2024

This PR has been very helpful for a project I'm working on — thanks!

I did notice that SharedAccessSignature.sas_url/3 hasn't been modified to accept config overrides.

@noaccOS noaccOS force-pushed the feat/multiple-azure-instances branch from 34fa38d to 19da24e Compare May 1, 2024 14:29
@noaccOS
Copy link
Contributor Author

noaccOS commented May 1, 2024

I did notice that SharedAccessSignature.sas_url/3 hasn't been modified to accept config overrides.

I implemented the same logic for the above function, but in doing so I have made the container optional (ie it defaults to Config.default_container like everywhere else)

Allow passing connection parameters together with the container to blob
functions.

Values from the parameters are prioritized over default configuration.
@noaccOS noaccOS force-pushed the feat/multiple-azure-instances branch from 4590c69 to 7b028b3 Compare October 4, 2024 10:22
@mattpolzin
Copy link

@jakobht bumping this in case it fell through the cracks. no worries if you've just been too busy to take another look.

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.

Config as an interchangable value
5 participants