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/version order #73

Merged
merged 1 commit into from
Nov 4, 2022
Merged

Conversation

pacoxu
Copy link
Contributor

@pacoxu pacoxu commented Nov 4, 2022

#72 (comment)

The version sort by string. We may introduce the version.ParseSemantic from "k8s.io/apimachinery/pkg/util/version" to compare the version by semantic.

  • coredns is not using v prefix. To use the version.ParseSemantic, we have to apply the prefix before comparing.

@pacoxu pacoxu force-pushed the fix/version-order branch 2 times, most recently from ad7b05b to 64aed32 Compare November 4, 2022 02:39
@pacoxu
Copy link
Contributor Author

pacoxu commented Nov 4, 2022

I am not sure if this is a best practice.

  • kubeadm depends on coredns/corefile-migration
  • coredns/corefile-migration depends on k8s.io/apimachinery v0.25.3

@chrisohaver
Copy link
Member

I am not sure if this is a best practice.

  • kubeadm depends on coredns/corefile-migration
  • coredns/corefile-migration depends on k8s.io/apimachinery v0.25.3

The import also seems to have a large impact in go.sum. Maybe we can just do our own implementation in line, e.g. ...

	sort.Slice(vStrs, func(i, j int) bool {
		iSegs := strings.Split(vStrs[i], ".")
		jSegs := strings.Split(vStrs[j], ".")
		for k, iSeg := range iSegs {
			if iSeg == jSegs[k] {
				continue
			}
			iInt, err := strconv.Atoi(iSeg)
			if err != nil {
				panic(err)
			}
			jInt, err := strconv.Atoi(jSegs[k])
			if err != nil {
				panic(err)
			}
			return iInt < jInt
		}
		return false
	})

or similar. It doesn't have to be bullet proof.

@pacoxu pacoxu force-pushed the fix/version-order branch from 64aed32 to da0253b Compare November 4, 2022 15:42
@pacoxu
Copy link
Contributor Author

pacoxu commented Nov 4, 2022

Updated.

The dep is not changed now.

@chrisohaver chrisohaver merged commit 0521279 into coredns:master Nov 4, 2022
@pacoxu
Copy link
Contributor Author

pacoxu commented Nov 28, 2022

@chrisohaver would you trigger a new release for #70?

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