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

Cicero should check that the template version is a valid semver #499

Open
jeromesimeon opened this issue Dec 16, 2019 · 6 comments
Open
Assignees
Labels
1.0 Type: Bug 🐛 Something isn't working

Comments

@jeromesimeon
Copy link
Member

Describe the bug
Each Accord Project template has a version number that is assumed to be a valid semver (https://semver.org).

Currently the check for valid semver uses a coercion on the version for both Cicero and the template:

https://github.com/accordproject/cicero/blob/c348ab2ae36ee70c43cb0d26402450d7517a3e26/packages/cicero-core/src/metadata.js#L79-L86

This does not update the versions in the package.json which means applications relying on those to be proper semver numbers would have to do the same coercion, which isn't documented.

To Reproduce
Steps to reproduce the behavior:

  1. got to any existing template
  2. Change the template version to 1.0
  3. Call cicero archive
  4. See that it does not raise an error

Changing the template version to foo would raise an error.

Expected behavior
Reject non-valid semver versions in templates.

@jeromesimeon jeromesimeon added the Type: Bug 🐛 Something isn't working label Dec 16, 2019
@jeromesimeon jeromesimeon self-assigned this Dec 16, 2019
@jeromesimeon
Copy link
Member Author

Proposed resolution: make the check in Cicero stricter and remove the semver coercions in the code in the issue description.

@jeromesimeon jeromesimeon changed the title Cicero should check that the template version is a valid server Cicero should check that the template version is a valid semver Dec 16, 2019
@jeromesimeon
Copy link
Member Author

Addendum: for the cicero version, it should check that this is a valid semver range not a valid semver version. I.e., it should allow 0.20.0 or ^0.20.0 but not ^foo.

@algomaster99
Copy link
Member

@jeromesimeon Is there anything left in this issue? I am asking since I see your pull request (#500 ) here.

@jeromesimeon
Copy link
Member Author

That's a good question. I don't think so -- I may have left it open at the time since it was pushed to a branch. I need to double check if this properly made it to the release-1.0 branch.

@jeromesimeon
Copy link
Member Author

d112bc6

I think it'll be released eventually. I'm closing this.

@jeromesimeon
Copy link
Member Author

Gah... it looks like this fix was merged into the wrong branch? Unclear, but I'm re-opening for investigation.

@jeromesimeon jeromesimeon reopened this Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Type: Bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants