Repository navigation
Conversation
GARNIX_DEFAULT_CONFIG points at a garnix.yaml the instance uses for repos that ship none, so an operator can change the default build set without recompiling. Unset keeps the built-in default.
| Just path -> do | ||
| eDecoded <- liftIO $ decodeFileEither path | ||
| case eDecoded of | ||
| Left e -> throw $ DecodeConfigError $ "GARNIX_DEFAULT_CONFIG (" <> cs path <> "): " <> cs (prettyPrintParseException e) |
There was a problem hiding this comment.
If the operator's file is broken, the repo owner is the one who gets blamed for it.
DecodeConfigError renders as Error decoding garnix.yaml: and maps to 400. So a repo with no garnix.yaml at all gets told its garnix.yaml failed to decode, complete with a path on your server, and a 4xx for something the owner can't do anything about.
Feels like it wants its own constructor: 500, and no mention of garnix.yaml.
| -- default. | ||
| instanceDefaultConfig :: (HasCallStack) => M GarnixConfig | ||
| instanceDefaultConfig = | ||
| liftIO (lookupEnv "GARNIX_DEFAULT_CONFIG") >>= \case |
There was a problem hiding this comment.
This reads instance-wide config from inside the per-build path, which bites twice.
If the path has a typo, decodeFileEither can't tell that apart from a bad parse, it just fails. So one wrong character in the env var and every repo without a garnix.yaml stops building, and nobody notices until things start going red.
Could this move up into withEnv and live on Env, the way #26 does it? Parsed once at boot, a bad path blows up right there, and the two PRs end up looking the same.
| liftIO (lookupEnv "GARNIX_DEFAULT_CONFIG") >>= \case | ||
| Nothing -> pure def | ||
| Just path -> do | ||
| eDecoded <- liftIO $ decodeFileEither path |
There was a problem hiding this comment.
Worth flagging that this hits the disk and re-parses on every build that has no config.
The cost is probably fine. What bugs me more is that the default can shift underneath you: edit the file while builds are running and two repos a minute apart quietly get different defaults, with nothing in the logs saying which one they got. Moving it to startup takes care of this too.
| Action "wild" ActionTriggerPush SharedResources True | ||
| ] | ||
|
|
||
| inM . aroundM_ suppressLogsWhenPassing . context "the instance default" $ do |
There was a problem hiding this comment.
These two cover unset and the happy path, but the risky side is the one that isn't tested.
I'd add a malformed file, pinning what the message actually says, and a path that doesn't exist, which is honestly the likelier mistake in practice.
2 participants
A repo with no
garnix.yamlgets a build set compiled into the binary. On a self-hosted instance that is the wrong place for it — the operator, not the upstream author, knows which attributes their builders can serve. A fork with no macOS builders wantsdarwinConfigurationsout of the default; changing that currently means a rebuild.GARNIX_DEFAULT_CONFIGpoints at a file ingarnix.yamlformat, used in place of the built-in default. Unset keeps current behaviour exactly, and a per-repogarnix.yamlstill wins.The resolution moves out of
getConfigintoinstanceDefaultConfigso it is callable without a repo checkout, which is what makes it testable. The parse error now names the file.