Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

[Documentation] Fix ambiguities of TypeScript and C# documentation for manifests #3643

Conversation

Batta32
Copy link
Collaborator

@Batta32 Batta32 commented Aug 31, 2020

Fix #3638

Purpose

What is the context of this pull request? Why is it being done?
The documentation TypeScript Update your Skill Manifest, C# Update your Skill Manifest and TypeScript Add your skill to a Virtual Assistant presents ambiguities described in the issue #3638.

Changes

Are there any changes that need to be called out as significant or particularly difficult to grasp? (Include illustrative screenshots for context if applicable.)

  • Explain the values of some placeholders
  • Add a publish.ps1 example
  • Link to deployment scripts document
  • Fix inconsistences of TypeScript manifests

Tests

Is this covered by existing tests or new ones? If no, why not?
-

Feature Plan

Are there any remaining steps or dependencies before this issue can be fully resolved? If so, describe and link to any relevant pull requests or issues.
-

Checklist

General

  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the appropriate tests
  • I have updated related documentation

Copy link
Contributor

@peterinnesmsft peterinnesmsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One area

```

> **Note**: `{YOUR_SKILL_URL}` is the endpoint URL where the Skill will receive the messages (e.g. `http://localhost:3979/api/messages`). Also, `{YOUR_SKILL_APPID}` is the `microsoftAppIp` value, the `{YOUR_SKILL_BOTWEBAPP_NAME}` is the `botWebAppName` and the `{YOUR_RESOURCEGROUP_NAME}` is the `resourceGroupName` that you can find in the `appsettings.json` file populated after the deployment of the Skill.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo here. Should be 'microsoftAppId'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lauren-mills, we already fixed the typo!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I think the Note makes it a lot easier to understand.

Copy link

@bhdzllr bhdzllr Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you deploy to Azure the Skill URL is also the name of the resource from Type "Web App-Bot", should we add this to the note?

Copy link

@bhdzllr bhdzllr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two different ports for locahost in this PR (3978, 3979) and in my example project it actually is 3980? What is the default port?

@Batta32
Copy link
Collaborator Author

Batta32 commented Sep 3, 2020

Thanks @bhdzllr for the feedback! We already updated the docs with the information you have provided.

@peterinnesmsft peterinnesmsft merged commit 0522b04 into microsoft:master Oct 16, 2020
@pr-triage pr-triage bot added the PR: merged label Oct 16, 2020
@Batta32 Batta32 deleted the feature/southworks/documentation/fix-ambiguities branch October 16, 2020 19:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Docs Documentation issue (missing needs updates, etc.) PR: merged PR: partially-approved PR: unreviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] Ambiguities in the docs (Project does not have a "wwwroot" folder; How to publish?)
4 participants