Merging features necessary for Juju Min Version #194

Merged
merged 14 commits into from Mar 8, 2016

Conversation

Projects
None yet
4 participants
Contributor

kat-co commented Mar 8, 2016

This introduces a dependency on github.com/juju/version.

meta_test.go
+ c.Check(err, gc.ErrorMatches, `invalid version "invalid-version"`)
+}
+
+// TestInvalidSeries ensures that invalid series values cause a parse error
@wallyworld

wallyworld Mar 8, 2016

Owner

comment need fixing, here and elsewhere

meta_test.go
+func (s *MetaSuite) TestInvalidMinJujuVersion(c *gc.C) {
+ _, err := charm.ReadMeta(strings.NewReader(dummyMetadata + "\nmin-juju-version: invalid-version"))
+
+ c.Check(err, gc.ErrorMatches, `invalid version "invalid-version"`)
@wallyworld

wallyworld Mar 8, 2016

Owner

can the error message be 'invalid min-juju-version "foo"' so the user knows what attribute is wrong

Owner

wallyworld commented Mar 8, 2016

LGTM with a couple of trivial fixes

Owner

wallyworld commented Mar 8, 2016

If we stick with having the attr as a version.Number we'd add schema support for it. May a todo would be good

Contributor

natefinch commented Mar 8, 2016

LGTM

Addressed review comments.
- Invalid versions specified in min-juju-version now refer to the field in the error.
- Removed unecessary comments from tests.
Contributor

kat-co commented Mar 8, 2016

$$merge$$

Contributor

jujubot commented Mar 8, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-charm

jujubot added a commit that referenced this pull request Mar 8, 2016

Merge pull request #194 from kat-co/feature-minver
Merging features necessary for Juju Min Version

This introduces a dependency on github.com/juju/version.

@jujubot jujubot merged commit c3eac73 into juju:v6-unstable Mar 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment