Add conditions as future replacement of requirements by ruflin · Pull Request #519 · elastic/package-registry

ruflin · 2020-06-17T13:50:12Z

This change adds conditions as part of the package manifest to align with other parts in the stack. Currently it adds the future of conditions and does not remove requirements yet. Requirements will be removed as soon as packages are updated.

elasticmachine · 2020-06-17T14:00:39Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #519 updated]
Start Time: 2020-06-30T20:03:57.836+0000
Duration: 8 min 16 sec

Test stats 🧪

Test	Results
Failed	0
Passed	161
Skipped	0
Total	161

ruflin · 2020-06-25T13:49:41Z

Throwing this in here as I found some additional conditions in an old file:

compatibility: [1.0.2, 2.0.1]
os.platform: [darwin, freebsd, linux, macos, openbsd, windows]

But these are probably more on the dataset level.

ruflin · 2020-06-25T18:53:22Z

testdata/package/example/1.0.0/manifest.yml


 owner.github: "ruflin"

+conditions:


@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?

+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.

Its a limited set of fixed conditions.

The conditions are joined with an AND this mean all condition predicates must return true.

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.

ruflin · 2020-06-29T11:06:42Z

Marked this as ready for review but it seems currently Github is flaky and has not picked up the most recent commits that I pushed :-(

As soon as elastic/package-registry#519 is merged, the mod file needs to be updated to the most recent version of the registry.

mtojek

Requirements will be removed as soon as packages are updated.

It would be faster to introduce such changes once new integrations are already merged. Otherwise, we need to track if all PRs are correctly rebased.

mtojek · 2020-06-30T07:28:15Z

testdata/generated/package/multiversion/1.1.0/index.json

    }
  },
+  "conditions": {
+    "kibana.version": "\u003e6.7.0"


I wonder why some fields are in such encoded format and some are not: kibana.version: ">6.7.0 <7.6.0"

The "problem" is the > char which gets converted. But I tested this quite some time ago and it works as expected even though it is converted.

mtojek · 2020-06-30T07:29:41Z

util/package.go


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


I suggest to rename this field, the difference is in capitals. How about kibanaConstraint or kibanaSemver?

Renamed to kibanaConstraint.

mtojek · 2020-06-30T07:32:48Z

Is there any meta-issue to link this PR?

This PR needs updating after elastic/package-registry#519 is merged.

ruflin · 2020-06-30T13:33:34Z

Meta issue tracking this change is in #517

This change adds `conditions` as part of the package manifest to align with other parts in the stack. Currently it adds the future of conditions and does not remove requirements yet. Requirements will be removed as soon as packages are updated.

ruflin · 2020-06-30T13:38:38Z

@mtojek PR updated with the reviews. Could you take a look again?

As soon as elastic/package-registry#519 is merged, the mod file needs to be updated to the most recent version of the registry.

ycombinator · 2020-06-30T19:52:43Z

util/package.go

+	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)


I think this should be p.Conditions.KibanaVersion?

ycombinator · 2020-06-30T19:59:17Z

util/package.go


-	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


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 } }

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

ycombinator · 2020-06-30T20:16:51Z

CHANGELOG.md

 * 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


Should requirements be deprecated?

I will be removed in a few hours ;-)

ycombinator

Left a note about the CHANGELOG. Code and test changes LGTM.

ph · 2020-07-01T12:38:32Z

post merge LGTM?

ruflin force-pushed the conditions branch from 81a6c40 to e1a36cd Compare June 25, 2020 18:47

ruflin commented Jun 25, 2020

View reviewed changes

ph assigned ruflin Jun 26, 2020

ruflin marked this pull request as ready for review June 29, 2020 11:06

ruflin mentioned this pull request Jun 30, 2020

[Meta] Planned changes to the manifest format #517

Closed

29 tasks

ruflin force-pushed the conditions branch 2 times, most recently from 6f0f3dc to ad1bca1 Compare June 30, 2020 07:24

ruflin changed the title ~~Conditions playground~~ Add conditions as future replacement of requirements Jun 30, 2020

ruflin requested a review from mtojek June 30, 2020 07:24

ruflin added a commit to ruflin/package-storage that referenced this pull request Jun 30, 2020

Rename requirement to conditions

1f64074

As soon as elastic/package-registry#519 is merged, the mod file needs to be updated to the most recent version of the registry.

ruflin mentioned this pull request Jun 30, 2020

Rename requirement to conditions elastic/package-storage#95

Merged

mtojek reviewed Jun 30, 2020

View reviewed changes

ruflin added a commit to ruflin/integrations that referenced this pull request Jun 30, 2020

Replace requirements by conditions

5a44cfa

This PR needs updating after elastic/package-registry#519 is merged.

ruflin mentioned this pull request Jun 30, 2020

Replace requirements by conditions elastic/integrations#133

Closed

ruflin added 2 commits June 30, 2020 15:35

update generated files

561177c

ruflin force-pushed the conditions branch from ad1bca1 to 561177c Compare June 30, 2020 13:36

update test

28ae667

ruflin requested a review from mtojek June 30, 2020 13:38

ruflin added a commit to elastic/package-storage that referenced this pull request Jun 30, 2020

Rename requirement to conditions (#95)

f333739

As soon as elastic/package-registry#519 is merged, the mod file needs to be updated to the most recent version of the registry.

ycombinator reviewed Jun 30, 2020

View reviewed changes

fix pr review

3698df9

ycombinator reviewed Jun 30, 2020

View reviewed changes

ycombinator approved these changes Jun 30, 2020

View reviewed changes

ruflin merged commit 33c9f9d into elastic:master Jun 30, 2020

ruflin deleted the conditions branch June 30, 2020 20:26

Conversation

ruflin commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

ruflin commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruflin commented Jun 29, 2020

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek commented Jun 30, 2020

Uh oh!

ruflin commented Jun 30, 2020

Uh oh!

ruflin commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

ph commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ruflin commented Jun 17, 2020 •

edited

Loading

elasticmachine commented Jun 17, 2020 •

edited

Loading