Style fixes and Python best practices #91
Conversation
|
@rayluo Could you review? |
Co-authored-by: Ray Luo <[email protected]>
|
@rayluo Good idea, I've updated the readme. |
|
Thanks, @pamelafox , this PR is getting better and better. :-) Are we still planning to modify the step 3 of README_B2C accordingly? I think the instruction of CLIENT_ID and CLIENT_SECRET will need to be changed to mention You can have the "azd-ify" change come in a different PR, so that we can conclude this one sooner. |
|
Yep, I still need to go through the B2C flow. Probably tomorrow morning. Also want to figure out if there's an article on Learn.com that links to that B2C readme. |
|
The B2C readme is referenced in these two articles: https://learn.microsoft.com/en-us/azure/active-directory-b2c/integrate-with-app-code-samples I'll go through the second article now to understand the old setup before I make adjustments. |
|
I've updated this PR with b2c related changes and sent a PR for the article: https://github.com/MicrosoftDocs/azure-docs-pr/pull/228856 I've now discovered an article in b2c docs that basically builds up the entire code sample: |
It makes total sense to update/simplify that doc. I believe that doc was written that way (i.e. "basically builds up the entire code sample") as part of the effort to provide step-by-step guidance. And that approach evolved into standalone chapterized tutorials afterwards. But their ongoing maintenance is underfunded. Now that this universal sample (which covers sign-in/sign-out, calling api, working in both AAD and B2C and another variation coming soon) is further simplified, perhaps we can simply remove all code snippets in that doc, and ask the readers to clone/download this sample instead. |
Co-authored-by: Ray Luo <[email protected]>
|
@rayluo I actually met with another advocate who is well versed in Powershell, and we ended up upgrading the Powershell scripts to a much newer module, based on the scripts used in the dotnet version of this sample. I've tested that flow and it seems to all work. The azure-docs-prs were both approved and merged, so if the Powershell changes are good with you, then feel free to merge! |
|
I do not use that powershell script myself. I can take a look into your new change in a high level within next couple workdays and then likely merge it in as-is.
This "fragmentation of helper scripts" sounds concerning. Will your future "azd-ify" PR address it? |
|
Yeah, I also don't use Powershell, that's why I asked another advocate to check my work. Here's the dotnet sample which the code is mostly from: https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/master/1-WebApp-OIDC/1-3-AnyOrgOrPersonal/AppCreationScripts/Configure.ps1 Good question re AZD-ifying. In that case, if the dotnet sample was also going to support azd, they would need similar Bicep code. So I think there'd be redundancy in both cases. With Bicep though, it's possible to store useful modules in a shared registry, so that's what we're hoping to do to avoid redundancy across similar samples. |
This PR updates the identity sample according to our review guidelines. The goal is to ensure that the code is Pythonic and also follows best practices.
To look for issues, I ran automated tools like black, ruff, isort, and pyupgrade, and kept the suggestions that were the most objectively Pythonic.
One significant change is that I introduced a .env file for the environment variables. This is what's done in many similar samples (like this one), and is a much safer practice. I nearly checked my secrets in multiple times when they were in app_config.py, so my team agreed that it made sense to move them into a non-checked in file. That change does require updating the article text, which I've done in this PR.
I will now leave inline comments on other changes to explain the reasoning.