From cf00365f84d4d59e8054a8de0918591e15041bbb Mon Sep 17 00:00:00 2001
From: Vincent Ambo <mail@tazj.in>
Date: Thu, 2 Jun 2022 17:42:56 +0000
Subject: [PATCH] feat(nix/buildkite): Validate available phases in extra steps

This will avoid things like extra steps being accidentally ignored
because of typos.

Change-Id: Ic4fa5925e42a7a449f89b4cde1510e216e91da6a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/5827
Reviewed-by: ezemtsov <eugene.zemtsov@gmail.com>
Tested-by: BuildkiteCI
---
 nix/buildkite/default.nix | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/nix/buildkite/default.nix b/nix/buildkite/default.nix
index 6a24fea07..5b83f9c66 100644
--- a/nix/buildkite/default.nix
+++ b/nix/buildkite/default.nix
@@ -13,6 +13,7 @@ let
     attrValues
     concatLists
     concatStringsSep
+    elem
     foldl'
     hasAttr
     hashString
@@ -165,7 +166,7 @@ rec {
 
           # Split extra steps by phase.
           splitExtraSteps = lib.groupBy ({ phase, ... }: phase)
-            (attrValues (mapAttrs (normaliseExtraStep overridable)
+            (attrValues (mapAttrs (normaliseExtraStep phases overridable)
               (target.meta.ci.extraSteps or { })));
 
           extraSteps = mapAttrs (_: steps: map mkExtraStep steps) splitExtraSteps;
@@ -287,7 +288,7 @@ rec {
   # Validate and normalise extra step configuration before actually
   # generating build steps, in order to use user-provided metadata
   # during the pipeline generation.
-  normaliseExtraStep = overridableParent: key:
+  normaliseExtraStep = knownPhases: overridableParent: key:
     { command
     , label ? key
     , needsOutput ? false
@@ -305,6 +306,15 @@ rec {
     let
       parent = overridableParent parentOverride;
       parentLabel = parent.env.READTREE_TARGET;
+
+      validPhase = lib.throwIfNot (elem phase knownPhases) ''
+        In step '${label}' (from ${parentLabel}):
+
+        Phase '${phase}' is not valid.
+
+        Known phases: ${concatStringsSep ", " knownPhases}
+      ''
+        phase;
     in
     {
       inherit
@@ -322,8 +332,6 @@ rec {
       # boolean API.
       #
       # To help users transition, emit warnings if the old API is used.
-      #
-      # TODO(tazjin): Validate available phases.
       phase = lib.warnIfNot (isNull postBuild) ''
         In step '${label}' (from ${parentLabel}):
 
@@ -339,7 +347,7 @@ rec {
         step definitions. Please remove the `postBuild` parameter from
         this step and instead set `phase = ${phase};`.
       ''
-        phase;
+        validPhase;
 
       prompt = lib.throwIf (prompt != false && phase == "build") ''
         In step '${label}' (from ${parentLabel}):