Refactor relevant classes to inherit from `ResourceBase`

[johlju]

The resources are already inheriting from DnsRecordBase which inherits from ResourcePropertiesBase so that we don't need to duplicate properties in every resource, so we already adhere to it. We should still keep that approach. It is like how SqlResourceBase is used. If changing to inherit from ResourceBase we should do that in separate PR and keep the inheriting logic so we don't duplicate resource properties. 🙂

This PR that handles HostNameAlias can either wait for that other PR or change it using current implementation.

@johlju Thank you for your patience and assistance!
Yes, I probably rushed with abandoning the common base class and inheriting directly from ResourceBase.
Using the SqlResourceBase class in SqlServerDsc is a good example worth following.

A minor concern regarding the DnsRecordBase class itself - its structure and functionality resemble ResourceBase ( but out-of-date in comparison with ResourceBase ).

Would it then be possible to proceed as follows?

• Keep existing resources inheriting from DnsRecordBase.
• For DnsRecordCname, which is being refactored, make it inherit from a new DnsResourceBase class and move the duplicated Properties we want to avoid into it.

Originally posted by @ahpooch in #298 (comment)

[johlju]

A minor concern regarding the DnsRecordBase class itself - its structure and functionality resemble ResourceBase ( but out-of-date in comparison with ResourceBase ).

It's because DnsRecordBase was the inspiration to create ResourceBase.

For DnsRecordCname, which is being refactored, make it inherit from a new DnsResourceBase class and move the duplicated Properties we want to avoid into it.

I don't like the idea of having yet another class. I think it is better in another PR to:

1. Make ResourcePropertiesBase inherit from ResourceBase
2. In DnsRecordBase (which inherits from ResourcePropertiesBase) remove methods that are no longer needed as ResourcePropertiesBase now have that functionality (since it inherits from ResourceBase)
3. Update all classes that inherits from DnsRecordBase as needed to handle changed functionality in parent classes.

[johlju]

When above is resolved resources like DnsServerCache, DnsServerDsSetting, DnsServerEDns, DnsServerRecursion and DnsServerScavenging should instead inherit from ResourcePropertiesBase and not ResourceBase - maybe there is also a need for additional classes to reduce duplication of properties. This was not possible before since the above wasn't fixed.

[dan-hughes]

When above is resolved resources like DnsServerCache, DnsServerDsSetting, DnsServerEDns, DnsServerRecursion and DnsServerScavenging should instead inherit from ResourcePropertiesBase and not ResourceBase - maybe there is also a need for additional classes to reduce duplication of properties. This was not possible before since the above wasn't fixed.

Not sure that will work, DnsServer DSC property type is different.

We need to consider the impending change in the preview ResourceBase.

It would be good to use preview ResourceBase, update tests then it will keep the PR's smaller.

[ahpooch]

@johlju @dan-hughes
As I understand it, the DnsServerDsc project may include DSC resources of the following types:

DNS records resources

Various DNS records and their scoped implementations:
DnsRecordA, DnsRecordAaaa, etc.
DnsRecordAScoped, DnsRecordAaaaScoped, etc.

DNS server settings

Various DNS server configurations:
DnsServerCache, DnsServerDsSetting, DnsServerEDns, DnsServerRecursion, DnsServerScavenging

DNS policies

DNS Policies
There are no examples of this type of resource in DnsServerDsc yet, but after gaining sufficient understanding of the project and implementation approaches, I would like to work on writing DSC resources for managing DNS Policies.
Preliminary resource names for implementation:
DnsServerQueryResolutionPolicy, DnsServerRecursionPolicy, DnsServerZoneTransferPolicy

Conclusion

Considering the above, would it be appropriate to try adhering to the following inheritance concept:
(DNS records resources classes) : DnsResourceRecordBase : ResourceBase
(DNS server settings classes) : DnsResourceSettingBase : ResourceBase
(DNS policies classes) : DnsResourcePolicyBase : ResourceBase

[ahpooch]

When above is resolved resources like DnsServerCache, DnsServerDsSetting, DnsServerEDns, DnsServerRecursion and DnsServerScavenging should instead inherit from ResourcePropertiesBase and not ResourceBase - maybe there is also a need for additional classes to reduce duplication of properties. This was not possible before since the above wasn't fixed.

Regarding the ResourcePropertiesBase class - I don’t understand its purpose, I mean, based on its name alone. 😅 I recognize that some base classes are needed to store common Parameters or hidden methods, but in this case, I would specifically argue against the very name of this class.

Here, I outlined my perspective on naming and the architecture of DSC resource types for DnsServerDsc.

[dan-hughes]

When above is resolved resources like DnsServerCache, DnsServerDsSetting, DnsServerEDns, DnsServerRecursion and DnsServerScavenging should instead inherit from ResourcePropertiesBase and not ResourceBase - maybe there is also a need for additional classes to reduce duplication of properties. This was not possible before since the above wasn't fixed.

Regarding the ResourcePropertiesBase class - I don’t understand its purpose, I mean, based on its name alone. 😅 I recognize that some base classes are needed to store common Parameters or hidden methods, but in this case, I would specifically argue against the very name of this class.

Here, I outlined my perspective on naming and the architecture of DSC resource types for DnsServerDsc.

Ignoring the name the class reduces the duplication of the DNS server property.

As you're starting out, you are looking at mature code with pretty robust testing and validation. It's hard and usually unnecessary to make large wholesale changes.

I was a new contributor about a year ago and it is hard to get going. There are the DscCommunity conventions in place which when applied across multiple projects make sense as everything looks and works the same.

My advice, pick a small bug or something marked good first issue and try and fix it.
Try not to do too much or get off track as then it makes PR's larger which means it takes more time to review and be merged.

[dan-hughes]

In this instance, there is a breaking change in the preview ResourceBase, it requires unit tests to be updated.

Once this is in place here, then other resources can start using the base class without needing a monster PR of changing all of the resources at once.

[johlju]

I would like to work on writing DSC resources for managing DNS Policies.
Preliminary resource names for implementation:
DnsServerQueryResolutionPolicy, DnsServerRecursionPolicy, DnsServerZoneTransferPolicy

@ahpooch Create issues for one of each of these (3 issues), there is an issue template for new resources - provide the properties you see is needed (see template) then we can discussion implementation in each issue. This is off-topic for this issue too, but since you mentioned it here it was easier to answer here. 🙂

[ahpooch]

I would like to work on writing DSC resources for managing DNS Policies.
Preliminary resource names for implementation:
DnsServerQueryResolutionPolicy, DnsServerRecursionPolicy, DnsServerZoneTransferPolicy

@ahpooch Create issues for one of each of these (3 issues), there is an issue template for new resources - provide the properties you see is needed (see template) then we can discussion implementation in each issue. This is off-topic for this issue too, but since you mentioned it here it was easier to answer here. 🙂

Despite the suggestion to create three separate issues for DNS Policies resources, I had to create a single consolidated issue to describe the overall approach to DNS Policies. This represents a completely new type of resource for DnsServerDsc.

At present, I categorize three distinct types of possible resources in DnsServerDsc:

• Records
• Settings (Also Legacy Settings could be viewed as a different type of resources)
• Policies

I also maintain the view that each of these resource types should architecturally have its own base class. However, I'm uncertain whether these base classes need to inherit from a single common parent class. If this is indeed necessary, the proposed name ResourcePropertiesBase doesn't seem particularly fitting to me - it might be worth considering renaming it.

Alternatively, it may make sense to:

• Create a new class with a more appropriate name (DnsResourceBase).
• Inherit from it for new resources.
• Gradually refactor existing resources to follow common practice.

In any case, ResourcePropertiesBase currently consists solely of a single DscProperty (DnsServer). From this perspective, the discussion seems more about aesthetic considerations.

I would fully support the following inheritance structure (taking into account previous comments and the example from the SqlServerDsc module):

DNS records resources classes : DnsResourceRecordBase : DnsResourceBase : ResourceBase
DNS server settings classes : DnsResourceSettingBase : DnsResourceBase : ResourceBase
DNS policies classes : DnsResourcePolicyBase : DnsResourceBase : ResourceBase
optionally:
DNS server legacy settings classes : DnsResourceLegacySettingBase : DnsResourceBase : ResourceBase

This discussion also relates to:
DnsServerDsc: The base classes need to be documented #216

[dan-hughes]

The fact that it's a separate resource type does not really matter. The 3 separate issues are to track the requirements, define the DSC properties and their types. They can be added as sub issues to a parent issue as required.

I would not get too hung up on what the class structure really looks like at the beginning, all that really matters is the resource name and properties as changes to any of those are usually breaking changes.

If a resource is refactored but uses the same properties then all is well as the end user/consumer does not need to change anything.

You cannot think of every possibility ahead of time, by building one resource you may not need any specific base class at the time. When the next resource that is built may allow the creation of a base class but you're heading down the premature optimization route.