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.

Sign up for GitHub

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

Jump to bottom

Add conditions as future replacement of requirements #519

Merged
merged 4 commits into from
Jun 30, 2020
Merged

Add conditions as future replacement of requirements #519

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c4ef2d
Add conditions as future replacement of requirements
ruflin Jun 17, 2020
561177c
update generated files
ruflin Jun 30, 2020
28ae667
update test
ruflin Jun 30, 2020
3698df9
fix pr review
ruflin Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Add list of downloads to /search endpoint. [#512](https://github.com/elastic/package-registry/pull/512)
* Apply rule: first package found served. [#546](https://github.com/elastic/package-registry/pull/546)
* Implement package watcher. [#553](https://github.com/elastic/package-registry/pull/553)
* Add conditions as future replacement of requirements. [#519](https://github.com/elastic/package-registry/pull/519)

### Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should requirements be deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be removed in a few hours ;-)


Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "~7.x.x"
},
"requirement": {
"kibana": {
"versions": "\u003e=7.0.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/base/0.2.0/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
"license": "basic",
"categories": [],
"release": "ga",
"conditions": {
"kibana.version": "\u003e7.9.0"
},
"requirement": {
"kibana": {
"versions": "\u003e7.9.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/example/0.0.2/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
"logs"
],
"release": "beta",
"conditions": {
"kibana.version": "\u003e=6.0.0"
},
"requirement": {
"kibana": {
"versions": "\u003e=6.0.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/example/1.0.0/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "~7.x.x"
},
"requirement": {
"kibana": {
"versions": "\u003e=7.0.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/foo/1.0.0/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
"metrics"
],
"release": "beta",
"conditions": {
"kibana.version": "\u003e=7.0.0"
},
"requirement": {
"kibana": {
"versions": "\u003e=7.0.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/longdocs/1.0.4/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "\u003e6.7.0"
},
"requirement": {
"kibana": {
"versions": "\u003e6.7.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/multiversion/1.0.3/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "\u003e6.7.0"
},
"requirement": {
"kibana": {
"versions": "\u003e6.7.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/multiversion/1.0.4/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "\u003e6.7.0"
},
"requirement": {
"kibana": {
"versions": "\u003e6.7.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/multiversion/1.1.0/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "\u003e6.7.0"
},
"requirement": {
"kibana": {
"versions": "\u003e6.7.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/generated/package/reference/1.0.0/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
"metrics"
],
"release": "ga",
"conditions": {
"kibana.version": "\u003e6.7.0 \u003c7.6.0"
},
"requirement": {
"kibana": {
"versions": "\u003e6.7.0 \u003c7.6.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/package/example/1.0.0/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ release: ga

owner.github: "ruflin"

conditions:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ph @michalpristas I initially was thinking about aligning the syntax with the constraints on the agent. But it only complicates things. The reason is that the available conditions here are very limited and have very specific checks. Adding additional "non yaml" syntax only complicates things. So here is what I'm proposing at the moment.

As each condition can only be used once I made it a key/value pair instead of an array.

One thing I think we should align is the naming. I landed on conditions as I think it fits best in all the places. WDYT about renaming it also in the agent?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on using "conditions" instead of constraints, which would align with dynamic input.

Now, I am Okayish with using key values pair, for what you are proposing.

  1. Its a limited set of fixed conditions.
  2. The conditions are joined with an AND this mean all condition predicates must return true.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, using key/value also mean you don't have to have the parser in Kibana and we don't have to share code in the registry.

So since everything is defined upfront I do not see a concern if the syntax arent the same, it's easier to start fixed and limited than to be dynamic all the way.

kibana.version: "~7.x.x"

requirement:
kibana:
versions: ">=7.0.0"
Expand Down
3 changes: 3 additions & 0 deletions testdata/package/reference/1.0.0/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ type: integration
owner:
github: "ruflin"

conditions:
kibana.version: ">6.7.0 <7.6.0"

requirement:
kibana:
versions: ">6.7.0 <7.6.0"
Expand Down
31 changes: 21 additions & 10 deletions util/package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type Package struct {
versionSemVer *semver.Version
Categories []string `config:"categories" json:"categories"`
Release string `config:"release,omitempty" json:"release,omitempty"`
Conditions *Conditions `config:"conditions,omitempty" json:"conditions,omitempty" yaml:"conditions,omitempty"`
Requirement Requirement `config:"requirement" json:"requirement"`
Screenshots []Image `config:"screenshots,omitempty" json:"screenshots,omitempty" yaml:"screenshots,omitempty"`
Assets []string `config:"assets,omitempty" json:"assets,omitempty" yaml:"assets,omitempty"`
Expand Down Expand Up @@ -97,6 +98,11 @@ type Requirement struct {
Kibana ProductRequirement `config:"kibana" json:"kibana,omitempty" yaml:"kibana"`
}

type Conditions struct {
KibanaVersion string `config:"kibana.version,omitempty" json:"kibana.version,omitempty" yaml:"kibana.version,omitempty"`
kibanaConstraint *semver.Constraints
}

type ProductRequirement struct {
Versions string `config:"versions,omitempty" json:"versions,omitempty" yaml:"versions,omitempty"`
semVerRange *semver.Constraints
Expand Down Expand Up @@ -150,7 +156,7 @@ func NewPackage(basePath string) (*Package, error) {
var p = &Package{
BasePath: basePath,
}
err = manifest.Unpack(p)
err = manifest.Unpack(p, ucfg.PathSep("."))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -185,8 +191,18 @@ func NewPackage(basePath string) (*Package, error) {

p.Downloads = []Download{NewDownload(*p, "tar")}

if p.Requirement.Kibana.Versions != "" {
p.Requirement.Kibana.semVerRange, err = semver.NewConstraint(p.Requirement.Kibana.Versions)
// If the new conditions are used, select them over the requirements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is duplicated code in this if-else to account for the new and legacy settings. Looks like the intent is to use the new setting, p.Conditions.KibanaVersion. So why not do something like this instead?

p.setKibanaVersion()
if p.Conditions != nil && p.Conditions.KibanaVersion != "" {
   ...
}

func (p *Package) setKibanaVersion() {
  if p.Conditions != nil && p.Conditions.KibanaVersion != "" {
    return
  }

  if p.Requirement.Kibana.Versions != "" {
    if p.Conditions == nil {
      // initialize p.Conditions
    }
    p.Conditions.KibanaVersion = p.Requirement.Kibana.Versions
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ycombinator The legacy code should only be around for 24-48h, so I was sloppy.

if p.Conditions != nil && p.Conditions.KibanaVersion != "" {
p.Conditions.kibanaConstraint, err = semver.NewConstraint(p.Conditions.KibanaVersion)
if err != nil {
return nil, errors.Wrapf(err, "invalid Kibana versions range: %s", p.Requirement.Kibana.Versions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be p.Conditions.KibanaVersion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
// TODO: remove legacy part
} else if p.Requirement.Kibana.Versions != "" {
p.Conditions = &Conditions{
KibanaVersion: p.Requirement.Kibana.Versions,
}
p.Conditions.kibanaConstraint, err = semver.NewConstraint(p.Requirement.Kibana.Versions)
if err != nil {
return nil, errors.Wrapf(err, "invalid Kibana versions range: %s", p.Requirement.Kibana.Versions)
}
Expand Down Expand Up @@ -253,16 +269,11 @@ func (p *Package) HasCategory(category string) bool {
func (p *Package) HasKibanaVersion(version *semver.Version) bool {

// If the version is not specified, it is for all versions
if p.Requirement.Kibana.Versions == "" {
if p.Conditions == nil || version == nil {
return true
}

if version != nil {
if !p.Requirement.Kibana.semVerRange.Check(version) {
return false
}
}
return true
return p.Conditions.kibanaConstraint.Check(version)
}

func (p *Package) IsNewerOrEqual(pp Package) bool {
Expand Down
82 changes: 82 additions & 0 deletions util/package_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
package util

import (
"log"
"testing"

"github.com/Masterminds/semver/v3"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -127,6 +129,86 @@ func TestValidate(t *testing.T) {
}
}

var kibanaVersionPackageTests = []struct {
description string
constraint string
kibanaVersion string
check bool
}{
{
"last major",
">= 7.0.0",
"6.7.0",
false,
},
{
"next minor",
">= 7.0.0",
"7.1.0",
true,
},
{
"next minor tilde",
"~7",
"7.1.0",
true,
},
{
"next minor tilde, x",
"~7.x.x",
"7.1.0",
true,
},
{
"next minor tilde, not matching",
"~7.0.0",
"7.1.0",
false,
},
{
"next minor tilde, matching",
"~7.0.x",
"7.0.2",
true,
},
{
"inside major, not",
"^7.6.0",
"7.0.2",
false,
},
{
"inside major",
"^7.6.0",
"7.12.2",
true,
},
}

func TestHasKibanaVersion(t *testing.T) {
for _, tt := range kibanaVersionPackageTests {
t.Run(tt.description, func(t *testing.T) {

constraint, err := semver.NewConstraint(tt.constraint)
assert.NoError(t, err)

p := Package{
Conditions: &Conditions{
kibanaConstraint: constraint,
},
}

kibanaVersion, err := semver.NewVersion(tt.kibanaVersion)
assert.NoError(t, err)

check := p.HasKibanaVersion(kibanaVersion)
log.Println(check)
assert.Equal(t, tt.check, check)

})
}
}

func BenchmarkNewPackage(b *testing.B) {
for i := 0; i < b.N; i++ {
_, err := NewPackage("../testdata/package/reference/1.0.0")
Expand Down