search

Azure-Samples / ms-identity-python-webapp

A Python web application calling Microsoft graph that is secured using the Microsoft identity platform
MIT License
293 stars 138 forks source link

Switch config from tenant_id to full authority #103

Closed rayluo closed 1 year ago

rayluo commented 1 year ago

This way, the sample is more generic to support authority of different cloud, or the upcoming CIAM product which will use authority https://tenant.ciamlogin.com.

@pamelafox , please:

pamelafox commented 1 year ago

This requires a slight doc update, yes. If you tag this as 0.6.0, then I think this branch should do it: https://github.com/MicrosoftDocs/azure-docs-pr/compare/main...pamelafox:azure-docs-pr:authority-url?expand=1

pamelafox commented 1 year ago

No changes needed to the B2C settings, right, since authority was already computed a different way?

rayluo commented 1 year ago

This requires a slight doc update, yes. If you tag this as 0.6.0, then I think this branch should do it: https://github.com/MicrosoftDocs/azure-docs-pr/compare/main...pamelafox:azure-docs-pr:authority-url?expand=1

Great. 0.6.0 is tagged now.

Perhaps you can go one step further to just reference/render the entire .env.sample there, so that we do not have to worry about "are there any relevant docs that need to be adjusted" in the future.

pamelafox commented 1 year ago

Yep, we could do that there. The potential drawback is that it will be a longer snippet since .env.sample contains comments, but that's probably okay. We would still want to update the code sample for that article to point at the changes, since that article has users actually downloading the repo - we want them in sync. We also need to update the highlight line numbers since the code lines have changed by one. I think the main advantage of the embedded code is making sure all the snippets are coherent with each other (even if they're not coherent with the article around, which could happen), and not having to worry about pasting code correctly into learn.com articles.

Basically, I think we always need to be mindful of the articles that reference a sample. For this one, the quick start's the most crucial article since it actively encourages cloning the repo.

pamelafox commented 1 year ago

Ah, I just realized this is merged, I'll send out the azure-docs-pr this afternoon (and change it to snippet the .env).

rayluo commented 1 year ago

The potential drawback is that it will be a longer snippet since .env.sample contains comments, but that's probably okay.

It should be OK, because those comments are all considered useful and suitable for the context.

We would still want to update the code sample for that article to point at the changes

Yea, I figure that would be the case. Then the question becomes "how/whether we know what docs need to be changed once this sample ships a new version".

I thought about maintaining a docs-to-be-changed.txt here, although that is not enforceable. New docs could pop up anytime and not necessarily updating that list here. Conceptually, it is the docs maintainer who would need to subscribe the release notification of this repo.

One thing I can do here is that I plan to change the versioning hint at the bottom right of this sample to refer to the version of this sample itself (instead of the version of the auth library). That way, it will be easier to spot an outdated document.

pamelafox commented 1 year ago

Yeah this is tricky, I've posted in the Learns chat to see if anyone has thoughts about it. One approach is to make sure every relevant article uses "ms-identity-python-webapp" somewhere inside it, which is typically the case when they use the code widget, so that makes it easier to find them with a search in azure-docs-pr.

Docs maintainers could also theoretically subscribe, but there are different maintainers based off B2C versus non-B2C. It auto-assigned 6 reviewers on the PR I just sent :)

Ohh, is that why there's a versioning hint? I don't know if any of the articles have screenshots (I can't remember any offhand) so I don't know if that will help the current articles.

rayluo commented 1 year ago

One thing I can do here is that I plan to change the versioning hint at the bottom right of this sample to refer to the version of this sample itself (instead of the version of the auth library). That way, it will be easier to spot an outdated document.

Ohh, is that why there's a versioning hint?

That was not the reason. :-) The version hint started as an optional eye candy, something like "powered by MSAL v1.2.3", in hoping that if a customer would send us a bug report with a screenshot, we would easily know - without asking - that which version of MSAL contains that bug. But that kind of bug report never arrives (MSAL and this sample have been very stable, quality-wise).

Now that this sample grows into a sophisticated project with its own versioning, it is time for us to repurpose the versioning hint to show the sample's version. That way, the customers will easily spot an updated document when they runs a newer version of the sample.

I don't know if any of the articles have screenshots (I can't remember any offhand) so I don't know if that will help the current articles.

I probably saw it once somewhere, but now I forget where it is. Never mind. We do not have to upgrade the screenshot. And people will eventually find it.

pamelafox commented 1 year ago

Ah okay cool, the screenshot thing does make sense, thanks for explaining!