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

Fix type of SRV target #1

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Fix type of SRV target #1

merged 4 commits into from
Dec 13, 2023

Conversation

rossabaker
Copy link
Contributor

The target type of an SRV record should be a hostname, not an int.

@rossabaker
Copy link
Contributor Author

rossabaker commented Dec 13, 2023

Is it unidiomatic to set an SRV record in networking.domains.subDomains? That's how I hit the type error. It's already fine if it's defined in extraConfig, which is covered by a unit test.

@Janik-Haag
Copy link
Owner

Uh thanks, I missed that. I'll take a look later

Is it unidiomatic to set an SRV record in networking.domains.subDomains?

No should be fine, I just didn't have a usecase yet.

That's how I hit the type error. It's already fine if it's defined in extraConfig, which is covered by a unit

Oh I thought so to but it's in fact not, because the attrset in utils/tests/zonefiles.nix is not the one passed to extraConfig but instead the a value of any key utils.domains.getDns would return. Did you actually try with extraConfig, I believe you should run into the same error because it is supposed to do the type-checking as well. I'll add a unit test checking both all record types for extraConfig and the nixosConfigurations.

I have to had of to $dayjob now, but I'll take a closer look in ~9-10 hours.

modules/records.nix Outdated Show resolved Hide resolved
@rossabaker
Copy link
Contributor Author

rossabaker commented Dec 13, 2023

Did you actually try with extraConfig,

I was sleepy and must have tried it against the corrected branch. I've added an example to the tests, which failed with the type error on target before the fix.

I'll add a unit test checking both all record types for extraConfig and the nixosConfigurations.

I haven't gone as far as this yet, but it's a good idea.

@Janik-Haag Janik-Haag merged commit 9f31187 into Janik-Haag:main Dec 13, 2023
1 check passed
Janik-Haag added a commit that referenced this pull request May 3, 2024
* Fix type of SRV target

* SRV target is not nullable

* Add SRV record to tests

---------

Co-authored-by: Janik <[email protected]>
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