[teraslice] Asset load dependency check #3822
Conversation
| minimum_versions?: { | ||
| teraslice: string; | ||
| }; |
There was a problem hiding this comment.
We probably want to think hard about what we name this and if it should be an object that can hold several versions or not.
There was a problem hiding this comment.
How would we prevent the opposite scenario? Where a new teraslice version breaks all the assets? For example a major change to job-components? This may be out of scope for this PR, or this just a tolerable defect?
There was a problem hiding this comment.
I'd say it's unavoidable since you can't know what breaking changes can be implemented in the future.
As long as this value won't need to be validated by convict that nested value looks OK. But every time I've tried to nest something like that I've gotten hung up when it goes through convict for validation (I think that was the issue). If we wanted to play it dumb and safe we could just do this: "minimum-teraslice-version".
busma13
changed the title
|
We really shouldn't be adding large binary files to the repo, especially one this large. At this point, since you've pushed the branch it's already there. So leave it. |
| } | ||
| if (metadata.minimum_versions) { | ||
| const terasliceVersion = getPackageJSON().version; | ||
| if (semver.gt(metadata.minimum_versions.teraslice, terasliceVersion)) { |
There was a problem hiding this comment.
What happens if the asset contains:
minimum_versions: {
};
So metadata.minimum_versions.teraslice is undefined?
Maybe we should just go back to a single property, no nested thing.
There was a problem hiding this comment.
Yeah this would have thrown an error because semver.gt() won't take undefined as a parameter.
|
I think the single property makes more sense. Any other compatibility check can have it's own top level property. This also gives me a chance to use a smaller asset. |
This PR makes the following changes:
ref: #3685