-
Notifications
You must be signed in to change notification settings - Fork 71
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
TypeScript version of Contoso Product #79
Conversation
hey @garrytrinder this is ready to be reviewed and merged. CC @BobGerman |
cdf8ee7
to
375a892
Compare
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.
Apologies for the delay on this @rabwill,
Thanks for making the changes in from the previous comments, there are a few things that I think we need to address before we merge.
I was able to get this working fine in Teams, but when using it in Copilot, I was unable to go through the authentication flow (when I click on the Sign in
button) it does nothing.
|
||
## Minimal path to awesome | ||
|
||
### Prepare Teams toolkit for VS Code |
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.
We can remove this section as users get prompted when they press F5 to start debugging
|
||
> NOTE: If you can't use the SharePoint look book service, you can find the source files to create it manually in the [SharePoint look book repository](https://github.com/SharePoint/sp-dev-provisioning-templates/tree/master/tenant/productsupport) | ||
|
||
### Prepare Azure |
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.
We don't need this section, when the arm/deploy action runs it will check for whether AZURE_SUBSCRIPTION_ID=
exists, if it doesn't it prompts the user to select a subscription and either select or create a resource group in the VS Code UI.
- Create a new resource group with the name `rg-msgext-product-support-local` | ||
- Find the subscription id keep it ready for configuration in the project. AZURE_SUBSCRIPTION_ID= | ||
|
||
### Prepare and run project |
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.
Let's remove the extra whitespace
### Prepare and run project | |
### Prepare and run project |
|
||
### Test | ||
|
||
- In Microsoft Teams, open the M365 Chat app |
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.
We should change this to reference the Copilot app in Teams rather than the older M365 Chat app
- In Microsoft Teams, open the M365 Chat app | |
- In Microsoft Teams, open the Copilot app |
samples/.DS_Store
Outdated
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.
We should remove this file.
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.
I will need some help I cannot find this in my local :( @garrytrinder is it possible to remove this while merging?
"identity", | ||
"messageTeamMembers" | ||
], | ||
"webApplicationInfo": {"id": "${{BOT_ID}}", "resource": "api://${{BOT_DOMAIN}}/botid-${{BOT_ID}}" }, |
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.
Let's fix the formatting here so it's not on a single line
BOT_ID= | ||
TEAMS_APP_ID= | ||
BOT_DOMAIN= | ||
BOT_ENDPOINT= | ||
AZURE_SUBSCRIPTION_ID= | ||
AZURE_RESOURCE_GROUP_NAME=rg-msgext-product-support-local |
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.
We can remove these as they will be added and populated during debugging (F5)
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.
Let's be consistent with whitespace between the parameter objects.
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.
@garrytrinder can you check if this updated version is what you meant by whitespace consistency? Thanks
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.
Let's be consistent with whitespace.
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.
@garrytrinder can you check if this updated version is what you meant by whitespace consistency? Thanks
Thanks @garrytrinder I have addressed most of the issue you have raised except testing in copilot. I need to repro this before I can fix the issue. Thanks for your patience! |
… and installing PnP PowerShell
ede31ef
to
28b5dea
Compare
… and installing PnP PowerShell
No description provided.