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

Feat azd compatible #9

Merged
merged 17 commits into from
Jan 2, 2024
Merged

Conversation

john0isaac
Copy link
Contributor

@john0isaac john0isaac commented Nov 19, 2023

Hey @pamelafox,
I love ❤️ your work and thought it would be a good thing to help you make this amazing project azd compatible. ✨
You'll still have to run the migrations yourself after the application is deployed. 🚀

I'm going to leave the application live for 48 hours if you want to test it out here you are two links
Deployment only with azd:
https://john-translation-phone-gmbxoafd3rnxo-appservice.azurewebsites.net/
Deployment with azd and migrations from SSH:
https://testing-env-nemljovihggx6-appservice.azurewebsites.net/recent

In this pull request:

  • add instructions for deployment using azd in readme.
  • delete .azure folder to be created automatically using azd.
  • add module ai/cognitiveservices.bicep
  • fixed an environment variable error - you were using a different username for database creation and application setting
  • add cognitive service translator resource

@pamelafox pamelafox self-requested a review November 26, 2023 21:02
README.md Outdated Show resolved Hide resolved
Co-authored-by: Pamela Fox <pamela.fox@gmail.com>
infra/main.bicep Outdated Show resolved Hide resolved
infra/main.bicep Outdated Show resolved Hide resolved
src/translations.py Outdated Show resolved Hide resolved
Copy link
Owner

@pamelafox pamelafox left a comment

Choose a reason for hiding this comment

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

thank you!! cant remember why I didn't azd-ify, didnt expect a kind soul to do it for me. just a few changes requested.

@john0isaac
Copy link
Contributor Author

john0isaac commented Nov 27, 2023

DONE, I deleted my previous comment because I thought it was wrong but it's correct anyway you'll get it when you review the code now.

TL;DR They use two ways to get the key EITHER implementing it just like I previously did (following their implementation I found that they were doing the same thing I was doing) OR using the SDK in python code to authenticate the application to access the key from the KV directly which I implemented here.

Checkout the live application: https://test-env-final-t5awkbvbz57xg-appservice.azurewebsites.net/
As usual I will leave it for 48 hours.

I know that you might want to leave the core/security as is and do these things somewhere else but in that case, I will have to create another folder in the infra just for storing the keys in the key vault (just like Jon's folders).
Either way, I'm okay - just let me know your preference.

@pamelafox
Copy link
Owner

Thanks for all the changes! I will take a look soon.

@john0isaac
Copy link
Contributor Author

Hey @pamelafox,
I have had the opportunity in the past couple of weeks to interact with lots of biceps and I see that even if it's for one bicep file we have to use the app to specify any special config.

@pamelafox
Copy link
Owner

Let me know when you're ready for re-review, I have your branch checked out locally to do a test deploy.

@john0isaac
Copy link
Contributor Author

john0isaac commented Jan 1, 2024

@pamelafox everything is good to go!
It was working before when I requested your review.

I recently came across this amazing line that makes app service pull the secret from the key vault without storing it as plain text in the application configuration variables.

'@Microsoft.KeyVault(VaultName=${keyVault.outputs.name};SecretName=postgresAdminPassword)'

So, I was changing the code to work with it - which made some of the changes I made during the process of using a Key Vault unnecessary so, I removed stuff and tidied up everything for the latest update.

I learned a lot. Thanks and Happy New Year to you and your family!! ❤️

@pamelafox
Copy link
Owner

Great! I deployed it and tweaked the readme a bit to emphasize migrating the DB. Merging, thanks so much!

@pamelafox pamelafox merged commit 1756ff5 into pamelafox:master Jan 2, 2024
1 check passed
@john0isaac
Copy link
Contributor Author

Thanks!
I'm a Gold Microsoft Learn Student Ambassador and I really love volunteering and Python If there is anything you need help with or opportunities to collaborate LMK.

@pamelafox
Copy link
Owner

Great to know! I spend most of my time these days in https://github.com/Azure-Samples/azure-search-openai-demo/ and we'll be running a hackathon around it in Jan/Feb, hope you can participate in that! It should be a lot of fun. (And there may be a prize for best OSS contribution...)

@john0isaac
Copy link
Contributor Author

I definitely will!

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.

None yet

2 participants