-
Notifications
You must be signed in to change notification settings - Fork 94
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
Adds Bash option throughout the workshop #175
Conversation
…ers for consistency
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 18 out of 33 changed files in this pull request and generated no comments.
Files not reviewed (15)
- docs/aca/08-aca-monitoring/Program-Backend.API-dotnet9.cs: Evaluated as low risk
- docs/aca/10-aca-iac-bicep/03-ci-cd-azdo.md: Evaluated as low risk
- docs/aca/10-aca-iac-bicep/02-ci-cd-git-action.md: Evaluated as low risk
- docs/aca/00-workshop-intro/4-prerequisites.md: Evaluated as low risk
- docs/aca/11-aca-landing-zone/index.md: Evaluated as low risk
- docs/aca/03-aca-dapr-integration/index.md: Evaluated as low risk
- docs/aca/10-aca-iac-bicep/index.md: Evaluated as low risk
- docs/aca/00-workshop-intro/3-dapr-integration.md: Evaluated as low risk
- docs/aca/00-workshop-intro/2-scenario-architecture.md: Evaluated as low risk
- docs/aca/00-workshop-intro/index.md: Evaluated as low risk
- docs/aca/00-workshop-intro/1-aca-core-components.md: Evaluated as low risk
- docs/aca/09-aca-autoscale-keda/index.md: Evaluated as low risk
- docs/aca/04-aca-dapr-stateapi/index.md: Evaluated as low risk
- docs/aca/08-aca-monitoring/index.md: Evaluated as low risk
- docs/aca/07-aca-cron-bindings/index.md: Evaluated as low risk
Comments suppressed due to low confidence (3)
docs/aca/02-aca-comm/index.md:282
- The environment variable should be consistently referred to as
BackendApiConfig__BaseUrlExternalHttp
when setting it via the command line.
Now we will need to update the Frontend Web App environment variable to point to the **internal** backend Web API FQDN. The last thing we need to do here is to update the Frontend WebApp environment variable named `BackendApiConfig:BaseUrlExternalHttp` with the new value of the *internal* Backend Web API base URL, to do so we need to update the Web App container app and it will create a new revision implicitly (more about revisions in the upcoming modules). We need to use the double underscore format to set the variable. The following command will update the container app with the changes:
docs/aca/01-deploy-api-to-aca/index.md:29
- The word 'intalled' is misspelled. It should be 'installed'.
Take note of the intalled .NET SDK versions and select the one with which you wish to proceed.
docs/aca/01-deploy-api-to-aca/index.md:192
- [nitpick] The variable name 'RANDOM_STRING' uses an underscore, while other variables use hyphens. It should be consistent.
$RANDOM_STRING=-join ((97..122) + (48..57) | Get-Random -Count 6 | ForEach-Object { [char]$_})
Hi @fdtmsft, Thank you for submitting this PR! I started to look through it. Could you please revert the canonical URL values to the bitoftech URLs? The reason why we use an external canonical for most content is that there is an agreement with @tjoudeh, who is the originator of the assets, to maintain canonicals to his blog. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdtmsft, I totally appreciate the effort and changes you have made. I know that takes much time.
The PR is rather big. Could I ask you to please tease apart the proposed changes in terms of just PowerShell and bash shell script from the meta changes you made for organization of the documents? I don't want to conflate the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I spot-checked a lot, albeit not everything, but what I saw was very consistent. Thank you so much, @fdtmsft!
Fixes #111 as well as adding Bash commands throughout the workshop for script snippets with the original being renamed PowerShell. This required some additional text modification for content to remain coherent.
It also updates the headers for consistency across the workshop using the same canonical URL as well as add a nav_order for sections having children sections (eg Module 10). Module 10 sections were also renamed for consistency with Introduction and Appendix.
@simonkurtz-MSFT @pankajagrawal16 @waelkdouh for review.