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.

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

fix: Ensure manifest.json not found in zip displays error #8261

Merged
merged 4 commits into from Jul 6, 2021

Conversation

EricDahlvang
Copy link
Member

@EricDahlvang EricDahlvang commented Jul 5, 2021

Description

  1. When a .zip is parsed, but the .json manifest is not found, display the exception
  2. When checking schema compat, check just the version and not the entire schema path

Task Item

fixes #8260
closes #8260

Screenshots

@EricDahlvang EricDahlvang changed the title fix: Ensure manifest.json not found in zip displays error fixex (#8260): Ensure manifest.json not found in zip displays error Jul 5, 2021
@EricDahlvang EricDahlvang changed the title fixex (#8260): Ensure manifest.json not found in zip displays error minor: Ensure manifest.json not found in zip displays error Jul 5, 2021
@EricDahlvang EricDahlvang changed the title minor: Ensure manifest.json not found in zip displays error #minor: Ensure manifest.json not found in zip displays error Jul 5, 2021
@EricDahlvang EricDahlvang changed the title #minor: Ensure manifest.json not found in zip displays error #minor Ensure manifest.json not found in zip displays error Jul 5, 2021
@cypress
Copy link

cypress bot commented Jul 5, 2021



Test summary

16 0 1 0Flakiness 0


Run details

Project Composer
Status Passed
Commit 3b52d45
Started Jul 6, 2021 11:10 PM
Ended Jul 6, 2021 11:17 PM
Duration 06:46 💡
OS Linux Ubuntu - 20.04
Browser Electron 89

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@EricDahlvang EricDahlvang changed the title #minor Ensure manifest.json not found in zip displays error fix: Ensure manifest.json not found in zip displays error Jul 5, 2021
@coveralls
Copy link

coveralls commented Jul 5, 2021

Coverage Status

Coverage decreased (-0.4%) to 56.258% when pulling 3b52d45 on eric/localSkillManifest into 031970c on main.

'https://schemas.botframework.com/schemas/skills/v2.2/skill-manifest.json',
];
return urls.includes(JSON.parse(content).$schema);
const versions = ['/v2.0/', '/v2.1/', '/v2.2/'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we should worry about a $schema field matching these shorter version strings but being something other than a longer URL (like the ones that used to be here)?

Copy link
Member Author

Choose a reason for hiding this comment

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

While the entire path is more correct, this seems less restrictive. I had a mistake in a manifest schema path, while the rest of the manifest was correct. I do not feel strongly about this though. If you object, we can skip this change.

Maybe we should be checking the $version, instead of the #schema path. Considering, our previous .schema had no version in the $id path: https://schemas.botframework.com/schemas/skills/v2.0/skill-manifest.json

"$id": "https://schemas.botframework.com/schemas/skills/skill-manifest.json",

Copy link
Member Author

Choose a reason for hiding this comment

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

@beyackle adjusted to endsWith '{v}/skill-manifest.json' and remove v2.0, considering the above comment

@hatpick hatpick merged commit f8d78c4 into main Jul 6, 2021
@hatpick hatpick deleted the eric/localSkillManifest branch July 6, 2021 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PVA as a Skill .zip manifest parsing issues
5 participants