Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not parse full config for commands that don't need it #179

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

unstubbable
Copy link
Collaborator

@unstubbable unstubbable commented Oct 4, 2023

Motivation

The cleanup job for the Feature Hub (using the purge command) currently fails with the following Zod validation error:

[
  {
    "code": "too_small",
    "minimum": 1,
    "type": "array",
    "inclusive": true,
    "exact": false,
    "message": "Array must contain at least 1 element(s)",
    "path": [
      "routes"
    ]
  }
]

This is because the routes are inferred from the built documentation. But when running the purge command, we don't actually need the routes, and we also don't want to build the docs just to avoid the config validation error.

Proposed Solution

For a couple of commands (including purge) we only need the domain name parts (hostedZoneName and sometimes aliasRecordName), so we can limit the parsing/validation to those fields, and thus implicitly allow an empty array of routes in this context.

Motivation

The [cleanup job for the Feature
Hub](https://github.com/sinnerschrader/feature-hub/actions/workflows/cleanup-stacks.yml)
(using the `purge` command) currently fails with the following Zod
validation error:

```
[
  {
    "code": "too_small",
    "minimum": 1,
    "type": "array",
    "inclusive": true,
    "exact": false,
    "message": "Array must contain at least 1 element(s)",
    "path": [
      "routes"
    ]
  }
]
```

This is because the routes are inferred from the built documentation.
But when running the `purge` command, we don't actually need the routes,
and we also don't want to build the docs just to avoid the config
validation error.

Proposed Solution

For a couple of commands (including `purge`) we only need the domain
name parts (`hostedZoneName` and sometimes `aliasRecordName`), so we can
limit the parsing/validation to those fields, and thus implicitly allow
an empty array of routes in this context.
Copy link
Collaborator Author

@unstubbable unstubbable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also be fine with merging the three files read-stack-config.ts, parse-stack-config.ts, and parse-domain-name-parts.ts into a single file called stack-config.ts. Or do you prefer it to be separated?

@clebert
Copy link
Owner

clebert commented Oct 16, 2023

I would also be fine with merging the three files read-stack-config.ts, parse-stack-config.ts, and parse-domain-name-parts.ts into a single file called stack-config.ts. Or do you prefer it to be separated?

I don't really like the ergonomics of the functions, they are very fragmented and therefore unintuitive to use, but right now I don't have a better suggestion either. But one function per file is usually my preferred structure, so it fits.

@clebert clebert merged commit c85843b into master Oct 16, 2023
2 checks passed
@clebert clebert deleted the partial-config-parsing branch October 16, 2023 07:38
unstubbable added a commit to feature-hub/feature-hub that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants