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

Added missing azure-tenant-id secret to databricks #331

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

damoodamoo
Copy link
Member

Found azure-tenant-id was expected in pipeline code, but missing from databricks secrets. This adds it in.

@damoodamoo
Copy link
Member Author

/test

@jjgriff93
Copy link
Member

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/UCLH-Foundry/FlowEHR/actions/runs/6260489355 (with refid a1e90952)

(in response to this comment from @damoodamoo)

@jjgriff93 jjgriff93 requested review from stefpiatek and removed request for tanya-borisova September 22, 2023 11:06
@jjgriff93
Copy link
Member

/test

@jjgriff93
Copy link
Member

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/UCLH-Foundry/FlowEHR/actions/runs/6273489000 (with refid a1e90952)

(in response to this comment from @jjgriff93)

@tanya-borisova
Copy link
Member

tanya-borisova commented Sep 23, 2023

Couldn’t we just add the secret through the config, same way as cognitive services key is added in?

Copy link
Member

@tanya-borisova tanya-borisova left a comment

Choose a reason for hiding this comment

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

Don’t think we need this

Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

LGTM, \test doesn't seem to pass, though seems unrelated to these changes. I presume we care about this passing so should fix it to get these changes in?

Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

... should've hit approve 🤦

Copy link
Member

@tanya-borisova tanya-borisova left a comment

Choose a reason for hiding this comment

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

@damoodamoo convinced me that this secret better lives upstream so I'm approving this :)

@jjgriff93
Copy link
Member

/test

@jjgriff93
Copy link
Member

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/UCLH-Foundry/FlowEHR/actions/runs/6417061708 (with refid a1e90952)

(in response to this comment from @jjgriff93)

@damoodamoo damoodamoo merged commit 777f5e6 into main Feb 13, 2024
1 of 2 checks passed
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.

4 participants