Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: Prevent schema from being overwritten #3215

Merged
a-b-r-o-w-n merged 5 commits into from
Jun 2, 2020
Merged

fix: Prevent schema from being overwritten #3215

a-b-r-o-w-n merged 5 commits into from
Jun 2, 2020

Conversation

@srinaath
Copy link
Contributor

@srinaath srinaath commented May 28, 2020 •
edited by a-b-r-o-w-n
Loading

Description

This PR addresses the following

  1. Custom schema supplied to Composer from PVA was overwritten on ejection. This has been sorted out
  2. The update-schema scripts(Bash and Powershell) have been added at the root. We have one .schema file at the root of the bot project which will be the source of truth
  3. Routing to default dialog if dialog id not specified in URL
  4. Routing schemaURL, name, description to the template if none specified in create protocol
  5. Fixes improper saving of huge schema file with write stream as only a part of schema file was saved

Task Item

refs #2322

Usages

update-schema.sh -runtime azurewebapp
update-schema.sh -runtime azurefunctions

a-b-r-o-w-n
a-b-r-o-w-n suggested changes May 29, 2020
View reviewed changes
runtime/dotnet/azurewebapp/Schemas/update-schema.sh
@@ -0,0 +1,24 @@
#!/bin/bash
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n May 29, 2020

Choose a reason for hiding this comment

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

We probably want a set -e for this script.

Copy link
Contributor Author

@srinaath srinaath Jun 1, 2020

Choose a reason for hiding this comment

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

Currently bfdialog has two bugs

  1. On error it deletes the sdk.schema file and leaves the user with no schema file
  2. It returns the 0 code despite error in merging schemas
    Both these are in GH. To overcome this I take a backup of the file and revert to the backup if the bfdialog process does not generate a new sdk.schema file.

Unless they fix both these issues adding set -e might be risky for us. Let's say they fix just the error code bug. That still leaves our shell script in an unstable state since on error, the script exits immediately. This leaves the user with no .schema file as the portion of the script where I revert back to the original schema file will be skipped. Let me know if Im missing something here

Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n Jun 2, 2020

Choose a reason for hiding this comment

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

That makes sense. Are those issues with the CLI being tracked? Is that what you mean by "Both these are in GH"?

Copy link
Contributor Author

@srinaath srinaath Jun 2, 2020

Choose a reason for hiding this comment

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

Correct.
microsoft/botframework-cli#806

@a-b-r-o-w-n a-b-r-o-w-n self-assigned this May 29, 2020
Srinaath Ravichandran added 2 commits June 1, 2020 15:38
Prevent schema from being overwritten
5745de9 
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Scripts updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Powershell and bash updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Update to readme

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Redirect routes to base bot and create protocols updated

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Prevent null initialization

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Refactored code

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

fail on alert

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Lowercasing schemas

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Stble commits
790b90c 
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>

Make sure exclude paths are handled

Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
@srinaath srinaath force-pushed the srravich/schema-overwrite branch from 3b799d4 to 790b90c Compare June 1, 2020 22:39
@coveralls
Copy link

coveralls commented Jun 1, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 42.676% when pulling 0713b72 on srravich/schema-overwrite into 4533b3a on master.

Update redirection to default dialog route
ed83b6e 
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
@srinaath srinaath requested a review from a-b-r-o-w-n June 1, 2020 23:45
a-b-r-o-w-n
a-b-r-o-w-n approved these changes Jun 2, 2020
View reviewed changes
runtime/dotnet/azurewebapp/Schemas/update-schema.sh
@@ -0,0 +1,24 @@
#!/bin/bash
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n Jun 2, 2020

Choose a reason for hiding this comment

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

That makes sense. Are those issues with the CLI being tracked? Is that what you mean by "Both these are in GH"?

@a-b-r-o-w-n a-b-r-o-w-n added the Approved to merge approved, waiting to be merged label Jun 2, 2020
@a-b-r-o-w-n a-b-r-o-w-n merged commit caa4514 into master Jun 2, 2020
@a-b-r-o-w-n a-b-r-o-w-n deleted the srravich/schema-overwrite branch June 2, 2020 15:05
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request Jun 3, 2020
Merged
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@srinaath
fix: prevent schema from being overwritten when ejecting (microsoft#3215
4540ef4 
)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@benbrown benbrown Awaiting requested review from benbrown benbrown is a code owner

@beyackle beyackle Awaiting requested review from beyackle

@boydc2014 boydc2014 Awaiting requested review from boydc2014 boydc2014 is a code owner

@carlosscastro carlosscastro Awaiting requested review from carlosscastro

@corinagum corinagum Awaiting requested review from corinagum

@cwhitten cwhitten Awaiting requested review from cwhitten cwhitten is a code owner

@luhan2017 luhan2017 Awaiting requested review from luhan2017 luhan2017 is a code owner

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

1 more reviewer

@a-b-r-o-w-n a-b-r-o-w-n a-b-r-o-w-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@a-b-r-o-w-n a-b-r-o-w-n

Labels

Approved to merge approved, waiting to be merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@srinaath @coveralls @a-b-r-o-w-n