CR for deployment branch (commit e3ad701) - Atidot/platform Wiki


Placeholder strings

Based on the way you're using string literals it seems like some of them are placeholders. Consider writing placeholderSecret = "tutorials/MyFirstTutorialSecret" and placeholderName = "hello-world", this would make it easier to understand which strings are permanent parts of the code and which are temporary.

Booleans in Deployment and DeploymentM.

I notice you're using a lot of booleans in these types, as well as in the functions you've defined here (e.g. in kiss and nsss). It would help if it was clarified by comments what these mean. If they keep track of whether a computation failed, maybe even define a type like so:

data DeploymentResult = DeploymentSuccess | DeploymentFailure.

This is structurally identical to the Bool type but it is much more clear what it means in the context of DeploymentM.


Type signatures

Berko mentioned to me that we should write our type signatures to enable easier commenting and Haddock documentation. E.g. your line 105 reads

scpW :: MonadIO io => Text -> Text -> Text -> io ()

Instead, everything after the :: should be aligned like so:

:: MonadIO io
=> Text
-> Text
-> Text
-> io ()

Variable names

In scpW I can follow most of the variable names except for pdns. I can understand what it does by reading the function, but pdns is too abbreviated for me to understand.

runAMI's run helper function

Line 67 reads next True. For the same reasons I mentioned with Platform.Deployment, it isn't 100% clear to me what this means.


Long lines

Many of the lines from line 80 to 102 are very long. Consider splitting these along multiple lines, possibly using helper functions. There are a couple of pairs of functions, like updatePrep and updateExec, or attachDockerFolder and attachDockerSecret, which duplicate code. Refactoring these might help with readability.


It's not clear to me where the values on lies 25-26 come from. Was this a configuration you used for testing? Or is this permanently part of the default?


Possible runtime exceptions

You call Data.Text.tail in getPublicDns and getInstanceId. This will generate a runtime exception if the input has a length of zero. Specifically, Data.Text.tail calls Prelude.error if it gets a length-zero input.

The functions getPublicDns and getInstanceId are both called in Platform.Deployment.Interpreter.AMI.runAMI (although getInstanceId is commented out). They are both called within a Control.Monad.Catch.bracket pattern at the beginning of runAMI. So a length-zero input to getPublicDns, for instance, will cause runAMI to jump into its fini' cleanup routine and then re-raise the exception. Is this the behaviour we want? Or should getPublicDns watch for a length-zero value and return a suitable output in that case? It's an honest question, I'm not sure what is the better choice here.