joergjo / contosoads-containerapp

Contoso Ads Azure Container Apps demo application
MIT License
7 stars 3 forks source link

Moving to Azure-Samples org #1

Open bradygaster opened 2 years ago

bradygaster commented 2 years ago

The repo looks fantastic. Great contribution, and appreciate you using one of the Microsoft company names so it'll make it easier to bring into the fold, so to speak. Adding @paulyuk from the Dapr team, as I know he'll be excited to see this content. Also adding @craigshoemaker, my partner from the docs team.

Here's a checklist of things I'd presume we need to do to pull this in:

All good with this?

joergjo commented 2 years ago

LGTM.

@bradygaster: Regarding the name: ContosoAds-on-AzureContainerApps is fine for me, but I'm open to other suggestions as well.

craigshoemaker commented 2 years ago

LGTM

@bradygaster: Are these samples being registered with the the Azure Samples browser as well?

bradygaster commented 2 years ago

@craigshoemaker i don't believe so - i'd love to be educated on how to do that. Want to do one of the existing repos and send me a PR, so i can learn from what you do and tweak the others?

joergjo commented 2 years ago

@bradygaster: "joergjo provides a brief summary for the samples page." - where do I need to provide the summary? Or is that by E-mail?

bradygaster commented 2 years ago

Sorry about the delay - was on vacation. Take a look here: https://docs.microsoft.com/en-us/azure/container-apps/samples

Essentially something like one of these paragraphs introducing the sample.

bradygaster commented 2 years ago

@joergjo - would you be interested in carving out that intro para?

joergjo commented 2 years ago

Yes, I'll post something here by tomorrow.

joergjo commented 2 years ago

@bradygaster Does that look OK?

bradygaster commented 2 years ago

Yep! That looks pretty good! I will get on this tomorrow and get it pulled in. Thanks SO much! cc @craigshoemaker FYI - let's also sync up on getting these into the samples browser.

bradygaster commented 2 years ago

@joergjo I've got the new azure-samples repo created, and I was in the process of moving stuff over but then I realized you don't have a github actions workflow file in here. Would you prefer I set that up for you or were you planning on adding it?

joergjo commented 2 years ago

@bradygaster : Yes, I had that in mind as a future feature. I won't have time to do so before next week, so I'd be happy if you could contribute that!

bradygaster commented 2 years ago

I will try to get to that and get it in, and update the readme as such, and will keep everyone up-to-date here.

joergjo commented 2 years ago

@bradygaster I'm happy to jump back in and tackle the missing GH action workflow. I've already added some updates to bring the sample up-to-date.

bradygaster commented 2 years ago

Appreciate that! I have an internal email about the repo I'm trying to figure out what to do about. Something triggered a review of it - nothing on your side, just a delay is all. I'll poke the issue to see if there's any way we can move forward. Also cc @paulyuk as he'll be excited about your use of Dapr in the sample.

joergjo commented 2 years ago

OK, should be done by early next week.

bradygaster commented 2 years ago

I'll also try to push through this odd internal axle I seem to have gotten wrapped around with getting this pushed out.

joergjo commented 2 years ago

@bradygaster

should be done by early next week said the guy who exhausted his Azure credits tow days later...

The workflow has been added. It follows the same approach as the other Azure Container Apps samples (e.g., https://github.com/Azure-Samples/dotNET-FrontEnd-to-BackEnd-on-Azure-Container-Apps), but simply deploys the Bicep files and uses my pre-built images available on Docker Hub.

bradygaster commented 2 years ago

@joergjo excellent thank you so much. Curious - did you sign the CLA? cc @craigshoemaker just to make sure you've signed the CLA and we're good to go there.

joergjo commented 2 years ago

I don't think I have (is it required for FTEs?). But how do I do that? AFAIK signing the CLA is triggered when submitting a PR, which in this case doesn't seem to apply.

bradygaster commented 2 years ago

OH never mind then!

bradygaster commented 2 years ago

Running the build now. Note - I had to make one change to the workflow file to get it to launch:


name: Deploy ContosoAds to Azure Container Apps

on:    
  push:
    branches:
      - deploy
    paths:
      - 'components/**'
      - 'deploy/**'
      - 'src/**'
      - '.github/workflows/**'
bradygaster commented 2 years ago

Found one more issue during deployment, looks like there's a missing password parameter expected during deployment. I'll look into it more deeply.

image

joergjo commented 2 years ago

See step 5 of the README - you have to create a secret called DB_PWD for PostgreSQL.

joergjo commented 2 years ago

Thanks for catching the other one - I ran into that as well while testing, and fixed it in my deploy branch on GH directly. And then deleted that branch 🫣

bradygaster commented 2 years ago

See step 5 of the README - you have to create a secret called DB_PWD for PostgreSQL.

AH i missed that.

bradygaster commented 2 years ago

Saw this happen once I deployed and the front-end was not responsive.

image

bradygaster commented 2 years ago

I looked more closely at the code and had an idea, and this aligns really closely with a theme I'm looking at closely and would love to try a solution out - mind if I change your data protection approach to use Key Vault and Azure Storage Blobs instead of the database?

joergjo commented 2 years ago

The DataProtectionKeys table is created by the container instance that executes the SQL script generated by EF Core. So if this didn't work, changing the data protection implementation won't help, since you will also miss the Ads table for the app itself.

Could it be that the Container App deployment was actually faster than the Container Instance deployment that runs the script? I've just looked at the Bicep code, and it doesn't use an explicit dependency between the app modules and the database module - I've never seen the Container Apps deploying that fast...

Another minor flaw I've found: The Bicep database module always sources the SQL script from this very repo: https://github.com/joergjo/contosoads-containerapp/blob/d6aa1cdbcada516510dc4ad02ac1b1f6749f9f37/deploy/modules/database.bicep#L126.

So I guess I need to extract another parameter here. I'll add these changes later today.

joergjo commented 2 years ago

I've added explicit dependencies from the Container Apps modules to the Database module and exposed the repo URL. The screenshots are also up to date.