Add two new validation constraints. #1175

aplanas · 2015-01-15T16:30:53Z

Add "platform" constraint:

"platform" => ['suse', /12.(0|1)/]

This will expect that the service have a platform equal to 'suse'
and a platform_version of 12.0 or 12.1
Add "exclude_platform" constraint:

"exclude_platform" => [/suse/, '11.3']

If the platform is a SLES11 SP3, the service will not be installed.

jsuchome · 2015-01-15T16:32:46Z

crowbar_framework/app/models/service_object.rb

+          :platform => platform,
+          :platform_version => version
+        }
+        platform.any? do |key, value|


I think you need to explictly return the result of this (also platform.all? in the second function)

aplanas · 2015-01-15T16:41:58Z

Return the result of all? and any?
Skip the evaluation if the role_constraint do not have the "platform" key

tboerger · 2015-01-15T16:45:44Z

👎 The client side validation is missing

aplanas · 2015-01-15T16:50:54Z

@tboerger Where is the client side validation code?

tboerger · 2015-01-15T17:04:30Z

crowbar_framework/app/models/service_object.rb

@@ -893,6 +943,28 @@ def validate_proposal_constraints(proposal)
        break
      end

+      if violates_platform_constraint?(elements, role)


Why is this part of the cluster constraint check?

It is not. I guess that you are tricked by the github collapse algorithm.

You are right

tboerger · 2015-01-15T17:06:47Z

I would prefer a hash for the constraint definition. The key is the platform and the value can be the specific version constrai t in form of a string, regexp or integer/float.

tboerger · 2015-01-15T17:09:25Z

You can find the javascript for clientside galidation at https://github.com/crowbar/barclamp-crowbar/blob/master/crowbar_framework/app/assets/javascripts/jquery/nodeList.js#L237

vuntz · 2015-01-16T10:52:23Z

@aplanas I'm not sure there's a way to express something like the following with your patch: "if platform is SLE, then SLE12 only; accept all non-SLE platforms"

jsuchome · 2015-01-16T10:56:03Z

@vuntz I think for that, we have "exclude_platform" => [/suse/, '11.3']

vuntz · 2015-01-16T11:03:00Z

@jsuchome well, it's "if platform is SLE, then not SLE11-SP3", which is slightly different than "if platfirm is SLE, then SLE12 only". But it might be good enough for now, indeed.

tboerger · 2015-01-16T11:09:01Z

But the pattern is still messy

aplanas · 2015-01-16T11:23:42Z

Yep, I think that "Accept SLES12 or anything that is not SLE" is approx. to "Exclude SLES11*" for this use case.

As @jsuchome this is something like { "exclude_platform" => ['suse', /9|(1[01].*)/] }

aplanas · 2015-01-16T11:42:42Z

I guess that to alleviate the messy regexp we can use @tboerger approach of using hashes instead of list.

  {
      "exclude_platform" => {
        "suse": "9",
        "suse": "10.*",
        "suse": "11.*"
      }
  }

But the semantic is the same : m

tboerger · 2015-01-16T11:50:21Z

That won't work. But regular expressions are fine. You can even negate the value of the regex. But hash keys have to be unique.

aplanas · 2015-01-16T11:54:08Z

Uau. Sometimes I amuse myself : (

Of course the hash is wrong. But we can use some lists here:

  {
      "exclude_platform" => [
        ["suse", "9"],
        ["suse", "10.*"],
        ["suse", "11.*"]
      ]
  }

tboerger · 2015-01-16T12:19:35Z

And why do you strictly rely on arrays? Something like this is IMHO absolutly fine. What do you think @bkutil?

{
  "exclude_platform" => {
    "suse" => /[^12]/
  }
}

Edit: And maybe something like this:

{
  "exclude_platform" => {
    "suse" => "< 12.0"
  }
}

jsuchome · 2015-01-16T12:30:53Z

I agree with use of hashes as @tboerger proposes. At least we do not have to convert arrays to hashes in validate functions

aplanas · 2015-01-16T12:46:03Z

I am not sure that I like the hash solution, are we going to allow this?

{
  "exclude_platform" => {
    "suse" => /[^12]/,
    "debian" => /.*/
  }
}

tboerger · 2015-01-16T13:36:45Z

Of course is that allowed.

aplanas · 2015-01-16T14:18:41Z

Change list with hashes

Add two new validation constraints.

Add "platform" constraint:

"platform" => {
  {
    'suse' => /12.(0|1)/,
    /debian/ => /.*/
  }
}

This will expect that the service have a platform equal to 'suse' and a platform_version of 12.0 or 12.1, or a debian.

Add "exclude_platform" constraint:

"exclude_platform" => {
  {
    'suse' => '11.3',
     /redhat/ => /.*/
  }
}

If the platform is a SLES11 SP3 or a redhat, the service will not be installed.

aplanas · 2015-01-16T16:19:21Z

Uhm, if I add regular expression, the javascript serializer do not work properly

tboerger · 2015-01-17T11:56:27Z

Please dont use regex for the hash keys.

aplanas · 2015-01-20T13:21:31Z

Add JS validators
Use strings to encapsulate regular expressions in JSON
Disable both constraints when cluster

    Add two new validation constraints.

    * Add "platform" constraint:

      "platform" => {
        'suse' => '/12.(0|1)/',
        'debian' => '/.*/'
      }

      This will expect that the service have a platform equal to 'suse'
      and a platform_version of 12.0 or 12.1, or a debian.

    * Add "exclude_platform" constraint:

      "exclude_platform" => {
        'suse' => '11.3',
        'redhat' => '/.*/'
      }

      If the platform is a SLES11 SP3 or a redhat, the service will not
      be installed.

    * Add constrains to JavaScript validator

      Because JSON can't contains regular expressions, the constraints
      needs to be expressed as strings.  If the string starts and ends
      with '/', the JavaScript and the Ruby part takes care of convert
      it into a Regexp.

jsuchome · 2015-01-20T13:49:17Z

crowbar_framework/app/models/service_object.rb

+      end
+
+      if violates_exclude_platform_constraint?(elements, role)
+        platforms = role_constraints[role]["platform"].map {|k, v| [k, v].join(' ')}.join(', ')


"exclude_platform"

aplanas · 2015-01-20T13:51:30Z

Fix error message for exclude_platform constraint

jsuchome · 2015-01-20T13:59:11Z

crowbar_framework/app/models/service_object.rb

+
+      if violates_exclude_platform_constraint?(elements, role)
+        platforms = role_constraints[role]["exclude_platform"].map {|k, v| [k, v].join(' ')}.join(', ')
+        validation_error("Role #{role} can't be used in #{platforms} platform(s).")


Shouldn't there be 'for xy platforms' in this message as well?

aplanas · 2015-01-20T14:08:27Z

Fix another typo in the error message
Rebase

tboerger · 2015-01-20T14:12:47Z

crowbar_framework/app/assets/javascripts/jquery/nodeList.js

@@ -359,6 +397,16 @@
    this.dataBag.removedOld = true;
  };

+  var equal_or_match = function(value, maybe_regexp) {


Please dont pullute the namespace. Make it as NodeList.prototype.equalOrMatch and call it with self.equalOrMatch()

aplanas · 2015-01-23T11:31:51Z

Do not coerce too much
Add tests cases suggested by @vuntz in Add two new validation constraints. #1175 (comment)

tboerger · 2015-01-23T11:36:20Z

@aplanas Sorry, i have done it wrong... https://github.com/chef/chef/blob/master/lib/chef/version_class.rb should be a good starting point how to parse the versions correctly. Split the platform version my . and compare the array.

aplanas · 2015-01-23T13:20:48Z

Use match function only for version numbers (JS and Ruby)
Implement a version comparator similar to https://github.com/chef/chef/blob/master/lib/chef/version_class.rb (JS and Ruby)

vuntz · 2015-01-23T13:35:13Z

crowbar_framework/app/assets/javascripts/jquery/nodeList.js

+
+      match = /^(\d+)$/.exec(version);
+      if (match) {
+        return [parseInt(match[1]), parseInt(match[2]), 0];


s/parseInt(match[2])/0/

vuntz · 2015-01-23T13:41:53Z

crowbar_framework/app/models/service_object.rb

+      end
+    end
+
+    op_value = maybe_regexp.scan(/^(>=?|<=?)\s*([.\d]+)$/)


For the record, I like the code flow of the js bits a bit better (where this regexp is only used when we're not in the /foo/ case.

I think that's not a big issue as the regex is not too hungry.

Can I maintain it here? I think that the eslif op_value is more clear here, and I can avoid multiple returns. I think that this is the idiomatic Ruby, but I am not sure.

@tboerger oh, it was not about performance, just about readability. If people prefer it here, then sure. It's just less readable (or well, more confusing) to me :-)

* Add "platform" constraint: "platform" => { 'suse' => '/12.(0|1)/', 'debian' => '/.*/' } This will expect that the service have a platform equal to 'suse' and a platform_version of 12.0 or 12.1, or a debian. * Add "exclude_platform" constraint: "exclude_platform" => { 'suse' => '11.3', 'ubuntu' => '> 10', 'redhat' => '/.*/' } If the platform is a SLES11 SP3 or a redhat, the service will not be installed. * Add constrains to JavaScript validator Because JSON can't contains regular expressions, the constraints needs to be expressed as strings. If the string starts and ends with '/', the JavaScript and the Ruby part takes care of convert it into a Regexp. * Add test cases

aplanas · 2015-01-23T14:04:12Z

Handle properly single variant of version in JavaScript

vuntz · 2015-01-23T15:21:27Z

Let's do it!

Add two new validation constraints.

jsuchome reviewed Jan 15, 2015
View reviewed changes

tboerger reviewed Jan 15, 2015
View reviewed changes

jsuchome reviewed Jan 20, 2015
View reviewed changes

tboerger reviewed Jan 20, 2015
View reviewed changes

vuntz reviewed Jan 23, 2015
View reviewed changes

aplanas mentioned this pull request Jan 23, 2015

Exclude Windows platforms. crowbar/barclamp-ceilometer#173

Merged

vuntz reviewed Jan 23, 2015
View reviewed changes

aplanas mentioned this pull request Jan 23, 2015

Exclude Windows platforms. crowbar/barclamp-cinder#225

Merged

vuntz added a commit that referenced this pull request Jan 23, 2015

Merge pull request #1175 from aplanas/master

1dba73b

Add two new validation constraints.

vuntz merged commit 1dba73b into crowbar:master Jan 23, 2015

This was referenced Jan 23, 2015

Exclude Windows platforms. crowbar/barclamp-nfs_client#3

Merged

Implement a proper comparison for versions. #1200

Merged

Add two new validation constraints. #1175

Add two new validation constraints. #1175

Conversation

aplanas commented Jan 15, 2015

Choose a reason for hiding this comment

aplanas commented Jan 15, 2015

tboerger commented Jan 15, 2015

aplanas commented Jan 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tboerger commented Jan 15, 2015

tboerger commented Jan 15, 2015

vuntz commented Jan 16, 2015

jsuchome commented Jan 16, 2015

vuntz commented Jan 16, 2015

tboerger commented Jan 16, 2015

aplanas commented Jan 16, 2015

aplanas commented Jan 16, 2015

tboerger commented Jan 16, 2015

aplanas commented Jan 16, 2015

tboerger commented Jan 16, 2015

jsuchome commented Jan 16, 2015

aplanas commented Jan 16, 2015

tboerger commented Jan 16, 2015

aplanas commented Jan 16, 2015

aplanas commented Jan 16, 2015

tboerger commented Jan 17, 2015

aplanas commented Jan 20, 2015

Choose a reason for hiding this comment

aplanas commented Jan 20, 2015

Choose a reason for hiding this comment

aplanas commented Jan 20, 2015

Choose a reason for hiding this comment

aplanas commented Jan 23, 2015

tboerger commented Jan 23, 2015

aplanas commented Jan 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aplanas commented Jan 23, 2015

vuntz commented Jan 23, 2015