Skip to content
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

[#355] Add Custom Version Matcher #508

Merged
merged 4 commits into from Dec 18, 2019
Merged

Conversation

@pedroMMM
Copy link
Contributor

pedroMMM commented Dec 2, 2019

Add Custom Version Matcher
Add Unit Tests
Add Integration Tests
Update Documentation

Copy link
Owner

aelsabbahy left a comment

This looks awesome! Thanks for adding unit tests with your commit.

I've requested a few changes, aside from that it looks good.

docs/manual.md Outdated Show resolved Hide resolved
integration-tests/test.sh Outdated Show resolved Hide resolved
@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Dec 3, 2019

Leaving this here for reference/potential future additions:

I should be able to test this PR more later this week.

I also wonder how much it changes things that I'm only getting "VERSION" out of rpm and ignoring epoch and others:

cmd := util.NewCommand("rpm", "-q", "--nosignature", "--nohdrchk", "--nodigest", "--qf", "%{VERSION}\n", p.name)

@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Dec 4, 2019

Just got back from my travels, decided to give this a quick go on my laptop to see how it works. The matcher should be renamed to clarify that it's a semver test. It seems to fail on around 7% of the debian packages:

I see the wheezy error you mention:

package:
  bash:
    installed: true
    versions:
      # This is what the command returns
      #- 4.2+dfsg-0.1+deb7u3
      version-gt: "4.4.0"
dgoss run -t aelsabbahy/goss_wheezy
INFO: Starting docker container
INFO: Container ID: e82d9a75
INFO: Sleeping for 0.2
INFO: Container health
INFO: Running Tests
Package: bash: installed: matches expectation: [true]
bash: version: Error: Expected a version or a list of versions.  Got:
    <[]string | len:1, cap:1>: [
        "4.2+dfsg-0.1+deb7u3",
    ]


Failures/Skipped:

bash: version: Error: Expected a version or a list of versions.  Got:
    <[]string | len:1, cap:1>: [
        "4.2+dfsg-0.1+deb7u3",
    ]

Total Duration: 0.007s
Count: 2, Failed: 1, Skipped: 0

Running this on all the wheezy packages in the docker test image gets the following result:

GOSS CRITICAL - Count: 243, Failed: 18, Skipped: 0, Duration: 0.096s

Can you clarify what you mean by this? That seems concerning if it's not parsing them correctly is it doing the correct thing when diffing non-semver strings?

I did a few quick tests since I had some time tonight and the constraints for go-version and semver are more rigid than the simple functions (GreaterThan...).

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

pedroMMM commented Dec 5, 2019

Can you clarify what you mean by this? That seems concerning if it's not parsing them correctly is it doing the correct thing when diffing non-semver strings?

I did a few quick tests since I had some time tonight and the constraints for go-version and semver are more rigid than the simple functions (GreaterThan...).

I have 2 examples from my unit tests:

"4.3.42-r".GreatherThan("4.0.0") // returns true
"4.4.019-1".GreatherThan("4.0.0") // returns true
go-version.Range("4.3.42-r > 4.0.0") // returns false
semver.Range("4.4.019-1 > 4.0.0") // errors out since no version segment can start with "0"

The go-version constraint fails because pre-release versions need to be pin down:

A constraint with a pre-release can only match a pre-release version with the same base segments.

With all of this in mind, I propose we follow your plan of naming the Matcher semver-constraint and use the semver package since it follows the specifications to the letter.

I will wait for your feedback on the plan before making changes.

@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Dec 5, 2019

semver-constraint + semver package sounds good to me.

This is a great start, really hoping other package managers get solved soon.

The semver matcher might not provide immediate value, but will be valuable in the near future once other work is done.

Preview of some upcoming work that will pair-up greatly with this in my opinion:

  • Migrating http/command/file to use gomega matchers. They were an older implementation and it's a little tricky to convert to gomega matchers because they're io.Reader. I don't want to switch them to arrays, since that could drastically increase the memory usage of goss (ex. someone doing validation on a 1GB log file). Basically gotta write a custom implementation of ContainElement that uses readers.
  • jmespath gomega matcher
  • Whenever a package type is introduced that strictly adheres to semver (not sure if there are many to be honest)
@milind69

This comment has been minimized.

Copy link

milind69 commented Dec 6, 2019

@pedroMMM pedroMMM force-pushed the pedroMMM:issue/355 branch 2 times, most recently from e66990e to 3d91e62 Dec 8, 2019
Copy link
Owner

aelsabbahy left a comment

Minor changes requested, aside from that this looks awesome and is good to go on my end. Let me know if you're all done with it.

Fun side note, doing a matching only test with this runs so fast the duration zero's it out =)

Total Duration: 0.000s
integration-tests/goss/goss-shared.yaml Show resolved Hide resolved
integration-tests/test.sh Outdated Show resolved Hide resolved
integration-tests/test.sh Outdated Show resolved Hide resolved
@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

pedroMMM commented Dec 9, 2019

The PR is ready on my side, thank you for all the feedback @aelsabbahy 👍

pedroMMM added 3 commits Dec 1, 2019
Add Custom Version Matcher 
Add Unit Tests
Add Integration Tests
Update Documentation
@pedroMMM pedroMMM force-pushed the pedroMMM:issue/355 branch from a3b92cb to 37314aa Dec 10, 2019
@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Dec 14, 2019

Planning on doing an in depth look/merge of this over the weekend.

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

pedroMMM commented Dec 14, 2019

Take your time, I just want to be sure it's ready when you are.

Copy link
Owner

aelsabbahy left a comment

Tested it locally, it looks great. I'm going to merge this, just one question:

matching:
  semver:
    content:
      - 1.9.9
      - 6.13.2
    matches:
      semver-constraint: ">1.0.0 <2.0.0 !=1.5.0"

Is there a way to check if only one element passes the constraint? If not, do you see the current implementation as easily changed to support that in a future PR? I would prefer composable gomega language if possible, since it allows it to be re-usable.

Real world use-case: package managers sometimes have multiple versions on a system (e.g. kernel) there might be a need to make sure that there's at least one package version installed that matches the constraint.

Just wanna hear your thoughts on this as it's something I would like to support around the same time deb/rpm support comes in.

My quick view/guess on this is, it might be possible with a simple gomega matcher that wraps semver-constraint. Something like:

matches:
  collection-any:
    semver-constraint: '...'

Since it seems you wrote the constraint in a way that works for a single string, which is awesome!


func toVersions(in interface{}) ([]*semver.Version, bool) {
if v, ok := toVersion(in); ok {
return []*semver.Version{v}, ok

This comment has been minimized.

Copy link
@aelsabbahy

aelsabbahy Dec 15, 2019

Owner

I think this solves my concern.. but want to get your take.

This comment has been minimized.

Copy link
@pedroMMM

pedroMMM Dec 15, 2019

Author Contributor

You are correct it will work with a single string or slice. I didn't consider this use case but as it stands it will work just as you described it.

@aelsabbahy

This comment has been minimized.

Copy link
Owner

aelsabbahy commented Dec 18, 2019

For what it's worth, this works just fine:

matching:
  semver:
    content:
      - 1.9.9
      - 6.13.2
    matches:
      contain-element:
        semver-constraint: ">1.0.0 <2.0.0 !=1.5.0"

Thanks for the contribution!

@aelsabbahy aelsabbahy merged commit 566b1b7 into aelsabbahy:master Dec 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pedroMMM pedroMMM deleted the pedroMMM:issue/355 branch Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.