Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Copilot Chat setup script fixes #1039

Merged
merged 3 commits into from
May 17, 2023
Merged

Copilot Chat setup script fixes #1039

merged 3 commits into from
May 17, 2023

Conversation

gitri-ms
Copy link
Contributor

Motivation and Context

  • Chocolatey won't install if %ProgramData%\Chocolatey exists but is empty
  • Requirement to specify tenant is confusing, and not actually necessary
  • Instructions for editing appsettings.json are unclear & easy to miss

Description

  • Check for choco.exe instead of just checking for install directory
  • Remove requirement to specify tenant in Setup.ps1 and Setup.sh. This aligns with the full Copilot Chat setup instructions, which just use the common endpoint and don't prompt the user for this.
  • Add appsettings.json to bulleted list of prerequisites for running these scripts

Contribution Checklist

@gitri-ms gitri-ms added bug Something isn't working docs and tests Improvements or additions to documentation PR: ready for review All feedback addressed, ready for reviews samples labels May 17, 2023
@hathind-ms hathind-ms merged commit 5f7d00a into microsoft:main May 17, 2023
19 checks passed
@gitri-ms gitri-ms deleted the cc-setup-readme branch May 19, 2023 00:42
shawncal pushed a commit to johnoliver/semantic-kernel that referenced this pull request May 19, 2023
@gitri-ms
Copilot Chat setup script fixes (microsoft#1039)
0ada4cc 
### Motivation and Context
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
- Chocolatey won't install if %ProgramData%\Chocolatey exists but is
empty
- Requirement to specify tenant is confusing, and not actually necessary
- Instructions for editing appsettings.json are unclear & easy to miss

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- Check for choco.exe instead of just checking for install directory
- Remove requirement to specify tenant in Setup.ps1 and Setup.sh. This
aligns with the full Copilot Chat setup instructions, which just use the
common endpoint and don't prompt the user for this.
- Add appsettings.json to bulleted list of prerequisites for running
these scripts

### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines
(https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] The code follows the .NET coding conventions
(https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions)
verified with `dotnet format`
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 馃槃
golden-aries pushed a commit to golden-aries/semantic-kernel that referenced this pull request Oct 10, 2023
@gitri-ms
Copilot Chat setup script fixes (microsoft#1039)
5039043 
### Motivation and Context
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
- Chocolatey won't install if %ProgramData%\Chocolatey exists but is
empty
- Requirement to specify tenant is confusing, and not actually necessary
- Instructions for editing appsettings.json are unclear & easy to miss

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- Check for choco.exe instead of just checking for install directory
- Remove requirement to specify tenant in Setup.ps1 and Setup.sh. This
aligns with the full Copilot Chat setup instructions, which just use the
common endpoint and don't prompt the user for this.
- Add appsettings.json to bulleted list of prerequisites for running
these scripts

### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines
(https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] The code follows the .NET coding conventions
(https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions)
verified with `dotnet format`
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 馃槃
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hathind-ms hathind-ms hathind-ms approved these changes

@adrianwyatt adrianwyatt adrianwyatt approved these changes

@microsoftShannon microsoftShannon Awaiting requested review from microsoftShannon

Assignees
No one assigned
Labels
bug Something isn't working docs and tests Improvements or additions to documentation PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gitri-ms @hathind-ms @adrianwyatt