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

Remove dependency on github.com/blang/semver #430

Closed
m1kola opened this issue Sep 26, 2023 · 3 comments
Closed

Remove dependency on github.com/blang/semver #430

m1kola opened this issue Sep 26, 2023 · 3 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.

Comments

@m1kola
Copy link
Member

m1kola commented Sep 26, 2023

Since #374 we have dependency on both github.com/blang/semver and github.com/Masterminds/semver. We introduced github.com/Masterminds/semver so that we can support certain version range syntax (#379).

I believe we should be able to get rid of dependency on github.com/blang/semver. As far as I see, the only place where we use it is here:

func InBlangSemverRange(semverRange bsemver.Range) Predicate[catalogmetadata.Bundle] {
return func(bundle *catalogmetadata.Bundle) bool {
bundleVersion, err := bundle.Version()
if err != nil {
return false
}
return semverRange(*bundleVersion)
}
}

This piece of code was migrated from old entity source code.

And we only use InBlangSemverRange here.

I believe we can replace InBlangSemverRange with InMastermindsSemverRange in this ocassion.

@m1kola m1kola added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Sep 26, 2023
@m1kola
Copy link
Member Author

m1kola commented Sep 26, 2023

@tmshort could you please take a look at this issue? I think we can remove github.com/blang/semver, but I have a feeling that I'm missing sometihng. I would really appreciate if you can confirm this or provide some background into why we did not remove InBlangSemverRange in #374/related PR.

@m1kola
Copy link
Member Author

m1kola commented Sep 26, 2023

Ah! I see this thread now. We need blang to support olm.package.required :(

Closing this one.

@m1kola m1kola closed this as completed Sep 26, 2023
@tmshort
Copy link
Contributor

tmshort commented Sep 26, 2023

We can't get rid of blang/semver since it's used to parse bundle versions (olm.package.required). The syntax different between blang and Masterminds is subtle, and we don't want to break bundle version checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.
Projects
None yet
Development

No branches or pull requests

2 participants