-
Notifications
You must be signed in to change notification settings - Fork 34
Version and simple manifest #14
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
Conversation
* `maintainer` (`Object`) | ||
* `name` (`String`) - the name of the maintainer | ||
* `email` (`String`) - the email address of the maintainer | ||
* `spec_version` (`String`) - the most recent version of the specification which this engine supports |
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.
I find this a little confusing. What am I specifying here?
- I work with exactly and only 0.0.1
- I work with versions up to 0.0.1
- I work with versions at least 0.0.1
- Something else?
I think we need to support a limited concept of version bounds here. I don't want to sign up for crazy bounds checking logic, but we need to allow the authors to clearly show if it's newer-than, older-than, or exactly the spec_version
they're specifying.
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.
@pbrisbin Cool, which one do you think we should do? I don't have strong opinions here.
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.
If we're intending to follow semantic versioning for the spec (which I'm in favor of), then I might suggest the following:
Keep the format as what you have, a single field with a single version value.
Interpret that value like the ~>
operator in gemspecs.
So:
spec_version: 0.0.1 means
I (claim to) support >= 0.0.1 && < 0.1.0
spec_version: 0.1 means
I support >= 0.1.0 && < 1.0.0
spec_version: 1 means
I support >= 1.0.0 && < *
(I think I have that right, correct me if I'm wrong)
WDYT?
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.
I actually think this should be a fixed version. It is the version that you are certifying your engine conforms to. If we have another, newer, spec version, it is incumbent on us to determine whether it is backwards compatible.
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.
I don't fully understand what you're saying, but if you can picture a future where engines specify an exact spec version they conform to and we don't end up in dependency hell as the spec evolves and cli/builder start checking and relying on said conformance, then 👍 from 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.
We can actually play this out using the include_path
feature as an example:
- Pretend it's a few weeks ago, before we built the
include_paths
feature - Pretend at that time the spec was v0.1
- Pretend we had this engines.json thing and all engines have
spec_version: 0.1
- We implement
include_paths
and deprecateexclude_paths
in SPEC - What happens?
- We actually want to stop supporting
exclude_paths
in CLI/Builder - What happens?
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.
The answer to the first "What happens?" is:
- We bump the spec version to 1.0 (not backwards compatible). The Code Climate CLI, during that time, supports both the 1.x and 0.x SPEC. When it runs an engine, it looks at the declared
spec_version
, and if it's0.x
it runs it with the old engine harness (probably emitting a warning)
The answer to the second "What happens?" is that we remove the 0.x
engine harness from the CLI, and it changes to refuse to run any pre-1.x
-designated engines
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.
Cool cool. This is the first I'm hearing of this versioned Engine Harness idea, and it sounds pretty non-trivial -- but OK, that all sounds fine on paper assuming we build said harness.
@mrb While you're doing this, can you also please add a |
|
||
## Manifest File | ||
|
||
All engines must include a `manifest.json` file at `/manifest.json`. This file, known as the manifest, includes information that is necessary for the analysis runtime. Here is an example of a manifest: |
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.
I'd call it engine.json
not manifest.json
. A manifest is a list of contents, which this includes, but it also includes other things. (In other words, this file contains a manifest, but is not a manifest itself)
* `name` (`String`) - the name of the maintainer | ||
* `email` (`String`) - the email address of the maintainer | ||
* `version` (`Integer`) - engine version | ||
* `spec_version` (`String`) - the most recent version of the specification which this engine supports |
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.
Still confused by "the most recent version...", why not just "the version..."?
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.
Good catch
|
||
## Engine Config File | ||
|
||
All engines must include a `engine.json` file at `/engine.json`. This file includes information that is necessary for the analysis runtime and metadata about the engine. Here is an example config: |
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.
grammar nit: "an engine.json
", not "a engine.json
"
Looks good. Ship it |
/c @brynary