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

Create informaten.com.gameserver_generic.json #580

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

Luxaaa
Copy link
Contributor

@Luxaaa Luxaaa commented Feb 9, 2025

Description

The new template should simplify the connection of custom domains to our gameservers (Minecraft, FimeM etc.) .

Type of change

Please mark options that are relevant.

  • New template
  • Bug fix (non-breaking change which fixes an issue in the template)
  • New feature (non-breaking change which adds functionality to the template)
  • Breaking change (fix or feature that would cause existing template behavior to be not backward compatible)

How Has This Been Tested?

Please mark the following checks done

RESOLVED:
I was not able to test the template in the online editor because it doesn't support variables for numeric fields like ttl or the SRV parameters.

The linter shows a warning for the SRV protocol variable saying that the value is invalid. But according to the example in this repository (README) and the spec, this sould be no problem-

  • Template file name follows the pattern <providerId>.<serviceId>.json

Example variable values

servicesubdomain: mc
protocol: tcp
service: minecraft

Copy link

github-actions bot commented Feb 9, 2025

Linter OK:

Linter result for informaten.com.gameserver_generic.json
{"level":"warn","template":"informaten.com.gameserver_generic.json","groupid":"","record":1,"type":"SRV","protocol":"%protocol%","code":"DCTL1015","dctl_note":"invalid value","time":1739113256}

Copy link
Member

@pawel-kow pawel-kow left a comment

Choose a reason for hiding this comment

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

Template has errors.
Please check with online editor, just by replacing not (yet) supported variables in ttl/port/protocol with static values.

BTW: I created an issue to make changes in the online editor (or in fact the library behind) but I must be honest here that likely I won't be able to tackle it before mid March.
Domain-Connect/DomainConnectApplyZone#14

"records": [
{
"type": "A",
"host": "%host%",
Copy link
Member

Choose a reason for hiding this comment

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

%host% is a special variable - please refer to the spec.
Please note that host is prepended to all hosts when the template is applied, so you'd rather want to put '@' here.

"priority": "%priority%",
"weight": "%weight%",
"port": "%port%",
"target": "%host%.%fqdn%.",
Copy link
Member

Choose a reason for hiding this comment

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

Also here please refer to the spec for %host% and %fqdn% special variables.
%fqdn% even includes %host% so it does not make any sense. I guess also here "@" is what is intended.

Please see in the online editor what it the result:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Thanks for the quick feedback. I fixed the template. Now (after replacing the variables) the online editor shows the records i want to have:

Bildschirmfoto_2025-02-09_13-52-41

Copy link
Member

Choose a reason for hiding this comment

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

If the intention is to have the same template applied several times with the same host and service values, but different randomnumber I would suggest to set the template to multiinstance (and read carefully the relevant part of the spec).
Also, due to some unclarity in the spec (see Domain-Connect/spec#117) identified recently, I suggest in this case to define A and SRV as two groupId and skip SRV group if one is already existing - otherwise the second instance of the template may render a conflict with the fist one if an identical record would be rendered.
A potentially easier alternative would be to define SRV as essential=OnApply. This would cope with the problem in other way without groups and prior lookup in DNS, however with a small drawback that someone may drop SRV record and leave A still in the zone (for providers which care about template integrity - see again spec).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the template again, now without a random number. It is not intended that a user adds the template multiple times for the same service and fqdn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bildschirmfoto_2025-02-09_16-04-37

@pawel-kow
Copy link
Member

@kerolasa can you take a look at this template?
It looks fine for me now, however linter throws a warning. Spec allows for variables in protocol of SRV if I read it right.

@pawel-kow pawel-kow enabled auto-merge February 9, 2025 16:04
@Luxaaa
Copy link
Contributor Author

Luxaaa commented Feb 10, 2025

@kerolasa can you take a look at this template? It looks fine for me now, however linter throws a warning. Spec allows for variables in protocol of SRV if I read it right.

Would it be better to add two different templates for tcp and udp instead of using a variable for protocol to avoid incompatibility with DNS Providers?

@kerolasa kerolasa self-requested a review February 10, 2025 17:39
Copy link
Collaborator

@kerolasa kerolasa left a comment

Choose a reason for hiding this comment

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

The SRV _proto field warning is just a warning. I might change linter to allow variable in that context as well.

https://github.com/Domain-Connect/dc-template-linter/wiki/DCTL1015

Other than that the template look acceptable to me.

Copy link
Member

@pawel-kow pawel-kow left a comment

Choose a reason for hiding this comment

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

Ok

@pawel-kow pawel-kow added this pull request to the merge queue Feb 10, 2025
Merged via the queue into Domain-Connect:master with commit 94088c2 Feb 10, 2025
2 checks passed
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