Skip to content

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

Merged
merged 5 commits into from
Sep 19, 2015
Merged

Version and simple manifest #14

merged 5 commits into from
Sep 19, 2015

Conversation

mrb
Copy link
Contributor

@mrb mrb commented Sep 18, 2015

* `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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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 deprecate exclude_paths in SPEC
  • What happens?
  • We actually want to stop supporting exclude_paths in CLI/Builder
  • What happens?

Copy link
Member

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's 0.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

Copy link
Contributor

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.

@wfleming
Copy link
Contributor

@mrb While you're doing this, can you also please add a version or engine_version field as an integer? We're going to need it for cache implementation, so we may as well add it now.


## 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:
Copy link
Member

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
Copy link
Contributor

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..."?

Copy link
Contributor Author

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:
Copy link
Contributor

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"

@mrb
Copy link
Contributor Author

mrb commented Sep 19, 2015

Okay, ready for final review @brynary @pbrisbin @wfleming

@brynary
Copy link
Member

brynary commented Sep 19, 2015

Looks good. Ship it

mrb added a commit that referenced this pull request Sep 19, 2015
Version and simple manifest
@mrb mrb merged commit 16aeeb0 into master Sep 19, 2015
@dblandin dblandin deleted the manifest branch March 15, 2017 15:06
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.

4 participants