search

Azure-Samples / graphrag-accelerator

One-click deploy of a Knowledge Graph powered RAG (GraphRAG) in Azure
https://github.com/microsoft/graphrag
MIT License
1.73k stars 275 forks source link

[BUG] ACR Autocreate may not actually test if ACR already exists #43

Closed timothymeyers closed 1 month ago

timothymeyers commented 3 months ago

Describe the bug Note, I believe the desired result is always achieved -- we end up using the existing ACR resources if it exists. This is mostly a bash logic / logging issue and absolutely a low level nitpick.

During subsequent runs of deploy.sh, the createAcrIfNotExists() function has an if [ !-z block that is not always run. If your bash session is reset between runs (e.g., my Codespace timed out and restarted!), the CONTAINER_REGISTRY_SERVER env variable may not be set any more after the first run.

To Reproduce

  1. Run deploy.sh without an ACR created (have one created for you using new autocreate logic). This will result in the CONTAINER_REGISTRY_SERVER variable being set for you.
  2. Open a new terminal session where CONTAINER_REGISTRY_SERVER is not set. (or just unset the variable).
  3. Run deploy.sh again. The output will say "Creating container registry" even though it is not actually creating a new one. The az deployment group create call on line 511 will return the existing server and everything proceeds as normal.

Expected behavior I would expect the script output to say it is checking for the server to exist and that an existing server was found.

jliberta commented 2 months ago

In your example, whether it's the first or any subsequent run, the "CONTAINER_REGISTRY_SERVER" variable is always unset. Given that it is an optional parameter, if you intend to use an existing registry from a prior run of deploy.sh or your subscription, then it's probably more appropriate to set this variable to your existing ACR login server.

If it's left unset, the Bicep file will simply generate the same unique ACR name on subsequent runs (Assuming you're using the same subscription and resource group) and will not apply any updates to your existing infrastructure. I'm not sure if there's a way to log this in Bicep.

It seems a bit counter-intuitive to check if a resource exists and to use it when the resource identifier is not provided.

timothymeyers commented 2 months ago

Nice catch and makes a lot of sense. I'm working on a PR (#106) to clean up the logging so it's a bit clearer what is happening.