feat(environments): allowed-times constraint #586

asher · 2019-11-06T23:01:35Z

No description provided.

luispollo · 2019-11-06T23:03:38Z

keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/DeliveryConfigControllerTests.kt

+        |            {
+        |              "hours": "6-18",
+        |              "days": "Monday-Friday",
+        |              "tz": "America/Los_Angeles"


Should the YAML payload have the tz field as well?

Yeah, we discussed this previously. It's necessary otherwise the server is going to interpret those hours as UTC

No, it's an optional field.

Also note the dynamic config to set a default TZ for windows other than UTC. This will be set to "America/Los_Angeles " so our users will only have to specify a TZ if they want something else.

luispollo · 2019-11-06T23:08:59Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/ConstraintEvaluator.kt

@@ -6,6 +6,24 @@ import com.netflix.spinnaker.keel.api.DeliveryConfig

 interface ConstraintEvaluator<T : Constraint> {

+  companion object {
+    inline fun <reified T> getConstraintForEnvironment(


Kotlin newbie question: does this need to be reified if you're not checking/using the type of T in the method?

Correct. Good spot.

Kotlin studies paying off! 😄

robfletcher · 2019-11-06T23:05:48Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/api/Constraint.kt

-@JsonDeserialize(`as` = DependsOnConstraint::class)
-sealed class Constraint
+@JsonTypeInfo(
+  include = JsonTypeInfo.As.PROPERTY,


This should be EXISTING_PROPERTY

robfletcher · 2019-11-06T23:06:18Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/api/Constraint.kt

+  Type(value = DependsOnConstraint::class, name = "depends-on"),
+  Type(value = TimeWindowConstraint::class, name = "time-window")


You should be able to omit the name since it's a constant in the types

robfletcher · 2019-11-06T23:07:58Z

keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/DeliveryConfigControllerTests.kt

+        |          "windows": [
+        |            {
+        |              "hours": "6-18",
+        |              "days": "Monday-Friday",


With this format can I specify something like "Monday, Wednesday and Friday"?

Or is that just 3 separate windows?

"and" isn't valid but you could specify "Monday, Wednesday, Friday"

you can do a list of ranges/individual days: https://github.com/spinnaker/keel/pull/586/files#diff-cb20a8e71cc1d6a198f43860b58d5696R149

@asher jinx.

robfletcher · 2019-11-06T23:08:37Z

keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/DeliveryConfigControllerTests.kt

+        |            {
+        |              "hours": "6-18",
+        |              "days": "Monday-Friday",
+        |              "tz": "America/Los_Angeles"


Yeah, we discussed this previously. It's necessary otherwise the server is going to interpret those hours as UTC

robfletcher · 2019-11-06T23:10:26Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+import java.util.TimeZone
+import java.util.TimeZone.getTimeZone


Shouldn't we use java.time.ZoneId here?

luispollo · 2019-11-06T23:18:31Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+import java.util.TimeZone
+import java.util.TimeZone.getTimeZone
+
+/**


Thanks for taking the time to document the class so well. ❤️

luispollo · 2019-11-06T23:26:16Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+        .toString()
+        .toLowerCase()
+      val todayShort = now.dayOfWeek
+        .getDisplayName(TextStyle.SHORT, Locale.getDefault())


Shouldn't this be Locale.US for consistency with the formatters up above?

I think it's the other way around, they should be Locale.default. Thanks for pointing out the discrepancy.

erikmunson

💯 This is some serious awesomeness. Nice work!

I had a couple specific comments, but also a more general observation: the way the constraint syntax works right now, you can't tell from just reading it whether the time windows are inclusion windows (only deploy during these times) or exclusion windows (don't deploy during these times). You can try to make assumptions (e.g. "it's a constraint so it's probably asserting things to approve, not deny"), but there's definitely room for interpretation as-is.

I sort of think about these constraints like unit test assertions — in theory you should be able to read them and get a sense of what the plain-english intent is, when possible. Any thoughts on how we can make the expected behavior more clear? Perhaps naming the windows field something more descriptive?

erikmunson · 2019-11-06T23:22:31Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+ * constraints:
+ *  - type: time-window
+ *    windows:
+ *      - days: Monday,Tuesday-Friday


Up in a test file I saw all lowercase for this value — looks like in this evaluator we're coercing any capitalization into lowercase?

I don't have a problem with that necessarily, but might be good to shoot for a standardized way of doing examples.

Or what if I add an additional window here that uses lower case? I want to show the flexibility in how days can be specified, but not sure how best to do that.

yeah maybe? I dunno. I suspect all these constraints will need really thorough docs to be successful. It's a detail and probably not super important tbh.

erikmunson · 2019-11-06T23:25:12Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+ *        hours: 11-16
+ *        tz: America/Los_Angeles
+ *      - days: Wed
+ *        hours: 12-14


Any opinions on 24hr time vs 12pm-2pm? Latter seems a little more readable, but not sure whether it's "better" from an overall perspective.

next you'll want us to use Fahrenheit

Kelvin, obviously.

Am and pm, how very quaint.

I'm delighted with 24hour

Alright alright, if everybody prefers 24h then let’s go with that. I’m coming from the perspective that for really any UI we and other engineers use day to day, times get mapped to 12h because that’s typically easier to visually parse at a glance. I generally see these YAML configs as more similar in their UX needs to UIs vs. actual programming code.

luispollo · 2019-11-06T23:31:50Z

...core/src/main/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluator.kt

+      ?: shortDayFormatter.parseUnresolved(days[1], ParsePosition(0))
+      ?: throw InvalidConstraintException("Failed parsing day range $this")
+
+    val intRange = IntRange(DayOfWeek.from(day1).value, DayOfWeek.from(day2).value)


What would happen here if you specify something backwards like thu-mon?

Probably not what the user would want.

Should we throw an InvalidConstraintException in that case, then?

Updated the behavior so wrap around hours and days work as one would reasonably expect.

luispollo · 2019-11-06T23:40:15Z

...src/test/kotlin/com/netflix/spinnaker/keel/constraints/TimeWindowConstraintEvaluatorTests.kt

+      name = "prod",
+      constraints = setOf(constraint)
+    )
+    private val previousEnvironment = Environment(


Philosophical question for the group: I thought time-window would be a type of constraint/requirement that would gate deployment into an environment regardless of a promotion flow between environments (I remember discussing this with Michael for the MD blog post), but the function in the evaluator interface is called canPromote. Can this be used with a single environment?

I think you should be able to control deployment into any environment with a time-window constraint. That applies if you have only 1 environment and you only want new builds deployed there on, let's say, weekdays.

I think that each constraint that is on an environment is evaluated, and they are && together. So if every constraint returns true to canPromote() then we would promote the artifact.

Thanks, Emily. I was just wondering if for the tests for this particular constraint (unlike for depends-on, which obviously needs 2 environments), we should then have a single environment in the fixtures.

I'll delete "staging" from the fixture if its causing confusion.

emjburns · 2019-11-06T23:55:15Z

keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/DeliveryConfigControllerTests.kt

+        |  - type: time-window
+        |    windows:
+        |      - hours: 6-18
+        |      - days: mon-fri


Should this yaml be:

windows: - hours: 6-18 days: mon-fri

(no dash by days) to mean that those go together as one window?

Ooh yes, otherwise it will be 2 separate windows

good catch, I'm so bad at yaml!

erikmunson · 2019-11-06T23:56:53Z

Another thought: the execution window UI for pipelines has three shortcut links that auto-select days for you: All, Weekends, and Weekdays.

Do any of those three seem like they'd be useful additions in the days field?

asher · 2019-11-07T00:00:41Z

@erikmunson I'll add the weekdays/weekends shortcuts for days, good idea. (All is the default when days are omitted)

luispollo

Other than the tiny things pointed out already, this looks great to me! Good stuff @asher!

emjburns · 2019-11-07T18:11:00Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/api/Constraint.kt

+data class TimeWindow(
+  val days: String? = null,
+  val hours: String? = null
+)


I think defining these here as strings means we get no validation when you submit a delivery-config. Do you think we could do some sort of validation like we do here, where that validation is pulled from the AllowedTimesConstraintEvaluator?

I'd like to know when I submit that I've spelled something wrong vs an error later :)

emjburns · 2019-11-07T18:16:19Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/exceptions/InvalidConstraintException.kt

+import java.lang.RuntimeException
+
+class InvalidConstraintException(
+  message: String


Probably should add constraint name here, so we know where the error is coming from?

class InvalidConstraintException( constraintName: String, message: String ) : RuntimeException("$constraintName: $message")

or something?

emjburns · 2019-11-07T18:17:58Z

...c/test/kotlin/com/netflix/spinnaker/keel/constraints/AllowedTimesConstraintEvaluatorTests.kt

+        }
+          .failed()
+          .isA<InvalidConstraintException>()
+      }


I'd like a test for when timezone is not set

emjburns · 2019-11-07T18:23:07Z

...re/src/main/kotlin/com/netflix/spinnaker/keel/constraints/AllowedTimesConstraintEvaluator.kt

+ *        hours: 12-14
+ *        tz: America/Los_Angeles
+ */
+@Component


This class is super clear. Thanks for adding great class docs, useful helper functions, keeping the core logic short and easy to reason about, and pulling the complex parsing out into smaller functions.

I struggled with this kind of calculation in Swabbie, and it's delightful to see something that reads well and makes sense here!

asher requested review from robfletcher, emjburns and luispollo November 6, 2019 23:01

luispollo reviewed Nov 6, 2019

View reviewed changes

robfletcher reviewed Nov 6, 2019

View reviewed changes

luispollo reviewed Nov 6, 2019

View reviewed changes

erikmunson reviewed Nov 6, 2019

View reviewed changes

luispollo reviewed Nov 6, 2019

View reviewed changes

emjburns reviewed Nov 6, 2019

View reviewed changes

luispollo approved these changes Nov 7, 2019

View reviewed changes

asher force-pushed the constraint branch 3 times, most recently from 81507a0 to 4f5b9f9 Compare November 7, 2019 17:42

feat(environments): allowed-times constraint

4d482d1

asher force-pushed the constraint branch from 4f5b9f9 to 4d482d1 Compare November 7, 2019 17:49

asher changed the title ~~feat(environments): time-window constraint~~ feat(environments): allowed-times constraint Nov 7, 2019

asher added the ready to merge Approved and ready for merge label Nov 7, 2019

emjburns reviewed Nov 7, 2019

View reviewed changes

emjburns approved these changes Nov 7, 2019

View reviewed changes

asher merged commit 7f513a8 into spinnaker:master Nov 7, 2019

asher deleted the constraint branch November 7, 2019 19:08

mergify bot added the auto merged label Nov 7, 2019

luispollo mentioned this pull request Nov 12, 2019

REQUEST: New Approver status for luispollo spinnaker/governance#64

Closed

6 tasks

		Type(value = DependsOnConstraint::class, name = "depends-on"),
		Type(value = TimeWindowConstraint::class, name = "time-window")

		import java.util.TimeZone
		import java.util.TimeZone.getTimeZone

feat(environments): allowed-times constraint #586

feat(environments): allowed-times constraint #586

Conversation

asher commented Nov 6, 2019

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

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

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

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

erikmunson left a comment • edited Loading

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

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

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

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

Choose a reason for hiding this comment

erikmunson commented Nov 6, 2019

asher commented Nov 7, 2019

luispollo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emjburns Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikmunson left a comment •

edited

Loading

emjburns Nov 7, 2019 •

edited

Loading