puppetlabs / opv

Ensure you know if systems don't work like they should - with Operational Verification and Validation resources
Apache License 2.0
10 stars 4 forks source link

Imperative names should be more declarative #8

Open binford2k opened 3 years ago

binford2k commented 3 years ago

Feedback provided by @reidmv in private slack DM.


I took a look at the OPV stuff and my first thought/reaction is around the language in the README of what look to be OPV-related resources.

It breaks the declarative state paradigm to use the word “check”. Check is not a desired state. It’s a verb. Leading with a verb makes the type name an imperative sentence. It’s mentally jarring to use the declarative resource syntax with imperative sentence resource type names.

This is a small change but it’s important. There’s a difference between this:

check_http { 'https://www.example.com':
  tries   => 3,
  timeout => 10,
}

and this:

http_check { 'https://www.example.com':
  ensure  => success,
  tries   => 3,
  timeout => 10,
}

I’m not sure this is exactly right, but I’m pretty sure I’m against imperative sentence type names.

DavidS commented 3 years ago

The rationale for this naming scheme is that the check_http resource is not actually configuring anything and really is doing an action at this point in the catalog evaluation.

@reidmv if that is not convincing, I'm happy to change it.

reidmv commented 3 years ago

Talking to @DavidS on Slack, we iterated on this concern a little.

Something that would alleviate it regardless of the resource name is to include an assertion parameter. In state-enforcing resources, the common assertion is "ensure". In a validation check resource, an appropriate assertion might be "expect".

http_check { 'app-basic-functionality':
  expect  => success,
  url     => 'https://www.example.com',
  tries   => 3,
  timeout => 10,
}
check_http { 'app-basic-functionality':
  expect  => success,
  url     => 'https://www.example.com',
  tries   => 3,
  timeout => 10,
}

While I'm still partial to http_check over check_http myself, I think the addition of a required expect parameter is A) requisite either way, and B) lowers the criticality of the resource title.