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

add the possibility for custom DNS records #1035

Merged
merged 5 commits into from
Jan 1, 2023

Conversation

christian-heusel
Copy link
Contributor

@christian-heusel christian-heusel commented Dec 2, 2022

This PR adds the code of madjam002@b9f05fc but patched into the correct file as the code location now has been updated.
I have run the latest headscale for the last month with this change and it works great! 🥳

My usecase is serving some internal applications such as grafana.vpn.my.domain and a have reverse proxy on the host that correctly maps the requests 👍🏻

I would be willing to add documentation, tests and more if needed, just let me know what you think of merging this before I invest the work ^^

  • read the CONTRIBUTING guidelines
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md

related to #762
Also notifying @madjam002 since the code was originally from him 😊

@juanfont
Copy link
Owner

juanfont commented Dec 4, 2022

Hi @christian-heusel, looks sensible :)

But we would need integration tests to verify that this works. Could you have a look at them?

@christian-heusel
Copy link
Contributor Author

Hey @juanfont could you point me to a PR which does a similar integration test so I get some idea about what I have to modify in order to get a working integration test for this feature? That would be really great!

So far I have got myself a working dev environment and spotted some code snippets that I probably have to modify.
See 2bd1edf for my current try xD

@juanfont
Copy link
Owner

@christian-heusel I left a comment in your branch :)

@juanfont
Copy link
Owner

@christian-heusel still very much interested into merging this.

Could you please add some documentation on the config-example file on how to use this (https://github.com/juanfont/headscale/blob/main/config-example.yaml) and an entry in the CHANGELOG.md?

We can leave the integration tests for a later PR. Happy to have a call/chat on them.

@christian-heusel
Copy link
Contributor Author

@juanfont Great to hear! Sorry for not getting back to you, I had some quite busy (but very nice) christmas days 🎅🏻 🎄
I can write the docs and example in the next days!

@christian-heusel
Copy link
Contributor Author

I can write the docs and example in the next days!

I now did a first take at the docs, have a look and let me know what you think!

@christian-heusel
Copy link
Contributor Author

@juanfont I also have made a changelog entry 👍🏻

Copy link

@megamorf megamorf left a comment

Choose a reason for hiding this comment

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

Suggested some minor style improvements.

docs/dns-records.md Outdated Show resolved Hide resolved
docs/dns-records.md Outdated Show resolved Hide resolved
docs/dns-records.md Outdated Show resolved Hide resolved
@christian-heusel
Copy link
Contributor Author

@megamorf Thanks for your suggestions and fixes! 🥳

Maybe just as a tip; you can use suggestions as described like so:
```suggestion
line to replace the highlighted one
```
Then the person which created the pr can just commit the suggestion! 👍🏻

@megamorf
Copy link

megamorf commented Jan 1, 2023

@megamorf Thanks for your suggestions and fixes! 🥳

Maybe just as a tip; you can use suggestions as described like so: suggestion line to replace the highlighted one Then the person which created the pr can just commit the suggestion! 👍🏻

Oh interesting. I've been working with GitHub Enterprise most of the time and haven't seen the feature there. That's a nice way to improve the overall feedback loop, thanks for sharing!

Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

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

Looks good!

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.

3 participants