chore(source): code cleanup

[ivankatliarchuk]

Description

No changes to business logic. Slicing this pull request #5186.

Moved few methods to shared/utils folder, added code re-use in some services. Increased code coverage for moved functions.

Target is to support for istio support v1alpha3 and v1. draft pull request is here gofogo#10

Checklist

• Unit tests updated
• End user documentation updated

[ivankatliarchuk]

/kind cleanup
/label tide/merge-method-squash

[mloiseleur]

Would you please rename source/annotations/annottaions.go to source/annotations/annotations.go ?

[ivankatliarchuk]

@mloiseleur Done

[mloiseleur]

/lgtm

[ivankatliarchuk]

Cool. Let me know if needs further split. Most changes - licence and test coverage.

[szuecs]

Shouldn't cloudflare use the annotations package const?

[ivankatliarchuk]

Changed. Cloudflare annotations defined in annotations package

[szuecs]

Get suggests that you get the alias from it, I would say has... would be more appropriate

[ivankatliarchuk]

Changed

[ivankatliarchuk]

Original code

external-dns/source/source.go

Line 185 in 277e8ca

func getAliasFromAnnotations(annotations map[string]string) bool {

[szuecs]

IMO it's clearly an error. Warning should have only use of communication deprecations, in my opinion.
Maybe we should decide how we use warnings, because I think during the last couple of months there's a little abuse of warnings.

[ivankatliarchuk]

Link to original code

external-dns/source/source.go

Lines 91 to 106 in 277e8ca

	func getTTLFromAnnotations(annotations map[string]string, resource string) endpoint.TTL {
	ttlNotConfigured := endpoint.TTL(0)
	ttlAnnotation, exists := annotations[ttlAnnotationKey]
	if !exists {
	return ttlNotConfigured
	}
	ttlValue, err := parseTTL(ttlAnnotation)
	if err != nil {
	log.Warnf("%s: \"%v\" is not a valid TTL value: %v", resource, ttlAnnotation, err)
	return ttlNotConfigured
	}
	if ttlValue < ttlMinimum || ttlValue > ttlMaximum {
	log.Warnf("TTL value %q must be between [%d, %d]", ttlValue, ttlMinimum, ttlMaximum)
	return ttlNotConfigured
	}
	return endpoint.TTL(ttlValue)

And pull commit where change was introduced 17e9637#diff-cf68f602fa7c20e5341f3b83054df68ade1586a144b1eae5347e0ac47096d3aa

The Warning makes to an extend.

• When parseTTL failes, there is a Warning that user should change TTL configuration
• Code uses default values, external-dns works.
• User could fix annotation or configuration when have time, and Warning will disapear

If it's an error;

• the code most likely should throw an error instead of setting up a default
• If there is a mistake in configuration, the external-dns is going to crash or behave differently

I need to capture all cases and provide an update for annotations proposal #5080

It was like that and I would rather keep it as is. In my opinion, I better capture this in annotation graduation proposal. Kind of all annotations should have validations, and validation should either be Error or Warning as example.

[szuecs]

Why define return variables in the signature if you don't use the feature?
Please drop them it's most off the time a code smell.

[ivankatliarchuk]

Will do. I was just moved things around, it was like that.

[ivankatliarchuk]

current master

external-dns/source/source.go

Line 115 in 277e8ca

func parseTTL(s string) (ttlSeconds int64, err error) {

[ivankatliarchuk]

I actually think the error is used on the client.

If an error, provide default

ttlValue, err := parseTTL(ttlAnnotation)
if err != nil {
   return ttlNotConfigured
}

[ivankatliarchuk]

Pls suggest what to do. Another option is parseTTLOrDefault

[szuecs]

Why rename? It reads simpler if you don't alias the package import here.

[ivankatliarchuk]

Removed antns. The main reason was - annotations is quite a common package name in most of the projects.

[ivankatliarchuk]

Example method attributes

external-dns/source/source.go

Line 247 in 277e8ca

func getTargetsFromTargetAnnotation(annotations map[string]string) endpoint.Targets {

. There was shadowing. So renamed to input

[szuecs]

Drop the whitespace here

[ivankatliarchuk]

Done

[szuecs]

Source should not import cloudflare.

[ivankatliarchuk]

Removed import

[szuecs]

Why redefining if we can just use the annotations package?

[ivankatliarchuk]

Fixed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mloiseleur. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[szuecs]

I think it's a great idea!
There are some smells I would like to see fixed, but I think it's rather small effort to do.
Another not commented thing is that in general I think a "utils" package is most often a code smell. I know Kubernetes project did it everywhere but that's not a good excuse. If it doesn't create a dependency cycle I would prefer to put these functions into source package.

[ivankatliarchuk]

Changed as per review. I think the utils is quite common pattern. Example go-sdk or helm. This most likely more like a preferred code style. Agree that better to create a specific packages, example strings, parse and other stuff. I have no strong opinion about that.

Let me know what else to change

[ivankatliarchuk]

Probably worth to share once again the motivation of why some of the functions moved to utils.

I have a follow-up pull request to address several Istio bugs and add support for v1alpha, v1beta, and v1 versions. While all code could reside in the
source folder, I've chosen to isolate the sources by source vendor and version. This approach enhances visibility into version-specific patterns and improvements, facilitates easier version support, and allows for more focused unit testing.

is not going to be exact folder structure, just an example at the moment