Skip to content

Conversation

hecflores
Copy link

I added code to support managed identity authentication.

I have not tested this! Dont complete right away - Can you walk me through the process for testing the changes and/or any patterns that I did that don't align to yours?

@RamonPage
Copy link
Owner

Hi! Thank you for your contribution! ✨

}
"""
@spec smart_connect() :: Client.t() | {:error, any}
Copy link
Owner

Choose a reason for hiding this comment

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

This looks out of scope for this feature. Could you move this piece to a separate PR for further discussion?

Client.t() | {:error, any}
def msi_connect(vault_name \\ nil) do
vault_name = get_env(:azure_vault_name, vault_name)
endpoint = System.get_env("IDENTITY_ENDPOINT")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest you move the IDENTITY_ENDPOINT and IDENTITY_HEADER env variables to the config files for each environment. This way you can leverage the get_env() function for them as well.

}
"""
@spec connect!() :: Client.t() | {:error, any}
Copy link
Owner

Choose a reason for hiding this comment

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

This seems out of scope as well. Also, although I understand the motivation for the ! function, the current connect() already raises exceptions.

Please open a PR or issue with this suggestion if you want to discuss more.

}
"""
@spec cert_connect!() :: Client.t() | {:error, any}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as connect!(). I'm happy to discuss more on a separate issue/PR.

@RamonPage
Copy link
Owner

This project is configured to expect 100% coverage. Could you include some tests?

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.

3 participants