-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add porter schema root command #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good starting point (noting your remark that this is not the final schema). My main concern is to what extent consumers will want to post-process the schema, e.g. converting the mixins
type to an enum
instead of free text. But I guess that can be down to the consumer - having an authoritative starting point is surely better than having to write the whole thing from scratch. One thing that might help with this is to move stuff out to a definitions
section, so consumers have a clear place to look for definitions, though I don't know whether this would really matter in practice. (I found it helped me get my thoughts straight when I was working with base+mixin schemas in the VS Code POC.) Such a change might be hard to make after release because tools that post-process the schema will depend on its layout.
In any case, thanks heaps for thinking about the tooling. I do feel this is on the right track even if it does take some iteration to get it playing nicely with the mixin scenario.
Switching back to WIP. After talking with @itowlson I'm going to have it work this way instead: $ porter schema
< dump a consolidate / merged JSON schema for the entire porter.yaml taking into consideration the mixins installed on the system> The other command |
a8e37a5
to
926717e
Compare
@itowlson Here is what the command looks like now. Let's ignore the helm mixin definition for now, I'm going to need to go back and fix it, since it's in another repository. I did do the exec mixin since it's in this repo. How well does this match what you expected and what VS Code requires? The list of definitions is dynamic based on what mixins are installed, and so is the mixins enum, and the refs under install. Once we have install working, I'll do upgrade and uninstall as well. {
"$schema": "http://json-schema.org/draft-07/schema#",
"additionalProperties": false,
"definitions": {
"exec.install": {
"properties": {
"description": {
"type": "string"
},
"exec": {
"properties": {
"arguments": {
"items": {
"type": "string"
},
"type": "array"
},
"command": {
"type": "string"
}
},
"type": "object"
}
},
"type": "object"
},
"helm.install": {
"additionalProperties": true,
"properties": {
"chart": {
"type": "string"
},
"name": {
"type": "string"
}
},
"type": "object"
}
},
"properties": {
"description": {
"type": "string"
},
"install": {
"anyOf": [
{
"$ref": "#/definitions/exec.install"
},
{
"$ref": "#/definitions/helm.install"
}
],
"type": "array"
},
"invocationImage": {
"type": "string"
},
"mixins": {
"items": {
"enum": [
"exec",
"helm"
],
"type": "string"
},
"type": "array"
},
"name": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
} If it's easier than reading the schema, I can give you a binary to try out locally. Just let me know. |
That looks like it's going the right way, but I'd consider refactoring the I think the way I had this in my POC evaluated to something like: // in the definitions section (the actual schema
// for install would be an array of this)
"install": {
"type": "object",
"properties": {
"description": { ... },
"outputs": { ... },
"exec": { "type": "object", "$ref": "#/definitions/exec-install" },
"helm": { "type": "object", "$ref": "#/definitions/helm-install" },
"oneOf": [
{ "required": "exec" },
{ "required": "helm" }
]
}
} This meant that (what I took to be) common stuff like Hope this makes sense - happy to explore in more detail if you want. I'm not trying to mandate what I came up with - in fact I prefer |
I just double checked and moving outputs under the mixin would not have any negative effect on porter. Description was being used by porter for logging progress such as "now executing the DESCRIPTION step". But that's ok to move too. It is tidier to move it all under the mixin. I'll try that, and keep the "anyOf" since you like that better. Hopefully this is getting closer to an end. Sorry it's taking so long! |
Sounds good - I'll load it up into VS Code and see how it plays out in terms of proferred popups and validation error messages. |
VS Code plays nicely with the proposed output - or at least no worse than it did with any of the other ways I've tried expressing the schema. The main difference I noticed was if I had both a I did need to make a few tweaks, but those were mostly relating to the Helm stuff which you hadn't done yet. We also need some {
"$schema": "http://json-schema.org/draft-07/schema#",
"additionalProperties": false,
"definitions": {
"exec.install": {
"properties": {
"description": {
"type": "string"
},
"exec": {
"properties": {
"arguments": {
"items": {
"type": "string"
},
"type": "array"
},
"command": {
"type": "string"
}
},
"type": "object",
"additionalProperties": false
}
},
"required": ["exec"],
"type": "object",
"additionalProperties": false
},
"helm.install": {
"properties": {
"description": {
"type": "string"
},
"helm": {
"properties": {
"chart": {
"type": "string"
},
"name": {
"type": "string"
}
},
"type": "object",
"additionalProperties": false
}
},
"required": ["helm"],
"type": "object",
"additionalProperties": false
}
},
"properties": {
"description": {
"type": "string"
},
"install": {
"type": "array",
"items": {
"anyOf": [
{
"$ref": "#/definitions/exec.install"
},
{
"$ref": "#/definitions/helm.install"
}
]
}
},
"invocationImage": {
"type": "string"
},
"mixins": {
"items": {
"enum": [
"exec",
"helm"
],
"type": "string"
},
"type": "array"
},
"name": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
} |
c1a24a9
to
ffa685a
Compare
ffa685a
to
90fd87b
Compare
90fd87b
to
247a31b
Compare
247a31b
to
ae044ec
Compare
@jeremyrickard Rebased for great glory |
Print out the manifest json schema
This lets us mock out calling executables from our unit tests. I'm working on making this not needed. But it may span multiple PRs as I split out the MixinProvider interface from the Porter app
* Use a function to create a box so we use the right name consistently * Save the box variable for reuse * Ensure we don't end up with nil pointers in any code paths
ae044ec
to
106ccfe
Compare
Okee, one more rebase to pick up the go generate PR and resolve some conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew! Awesome work on this. A few questions/comments at this stage for me...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think the stderr logging should get updated in a follow up PR.
Merge like this PR is the albatross that ruined the month of February. 🐦 |
Note: This isn't the full json schema. We will fill that in once we validate this these commands are plumb together (between an IDE, porter and the mixins) to help provide autocomplete/intellisense.
I'm following up on this PR with another that addsEDIT: After talking with @itowlson this is on hold, may not need a subcommand.porter schema --mixin NAME
but figured it would be easier to review incrementally.Closes #76
Here's all the things I found when shaving this yak that need to be split out into separate PRs most likely: