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

Zonemaster::LDNS and libldns versions #306

Merged
3 commits merged into from Feb 20, 2023
Merged

Zonemaster::LDNS and libldns versions #306

3 commits merged into from Feb 20, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 14, 2023

Purpose

Show which versions of Zonemaster-LDNS and NL NetLabs LDNS are being used.
Remove module versions.

Context

Addresses #181

Changes

CLI.pm and version() method.

How to test this PR

Verify that the following command returns 2 new lines LDNS version and libldns version:

$ zonemaster-cli --version
Zonemaster-CLI version v5.0.1                                                                                          
Zonemaster-Engine version v4.6.1     
Zonemaster-LDNS version 3.1.0                    
NL NetLabs LDNS version 1.8.3

@ghost ghost added T-Feature Type: New feature in software or test case description V-Minor Versioning: The change gives an update of minor in version. labels Feb 14, 2023
@ghost ghost added this to the v2023.1 milestone Feb 14, 2023
@ghost ghost requested a review from matsduf February 14, 2023 13:00
@matsduf
Copy link
Contributor

matsduf commented Feb 14, 2023

Show which versions of Zonemaster::LDNS and libldns are being used.

That is a good idea.

$ zonemaster-cli --version
CLI version:    v5.0.1
Engine version: v4.6.1
LDNS version: 3.1.0
libldns version: 1.8.3
...

However, I think that more updates should be done. Just as the issue points out the module numbers do not add anything. I suggest that they should be removed.

Secondly, we should use the full name of the Zonemaster components, especially it is problematic to refer to "LDNS" when we mean Zonemaster-LDNS since LDNS is a different code. Compare with how it is displayed when running the web GUI.

Thirdly, I think it would be better to refer to the NL NetLabs code as LDNS since that is what they call it. I suggest that "zonemaster-cli --version" outputs the following (and nothing else):

$ zonemaster-cli --version
Zonemaster-CLI version v5.0.1
Zonemaster-Engine version v4.6.1
Zonemaster-LDNS version 3.1.0
NL NetLabs LDNS version 1.8.3

@matsduf matsduf linked an issue Feb 14, 2023 that may be closed by this pull request
@ghost
Copy link
Author

ghost commented Feb 15, 2023

Updated to address your remarks.

@matsduf
Copy link
Contributor

matsduf commented Feb 16, 2023

I also fee this is "Minor", but technically, isn't this a change of API, i.e. "Major"?

@ghost
Copy link
Author

ghost commented Feb 20, 2023

I also fee this is "Minor", but technically, isn't this a change of API, i.e. "Major"?

Indeed. It was a "minor" change at first where only new lines were added. Since we remove and update the existing output, it becomes a "major" change.

@ghost ghost added V-Major Versioning: The change gives an update of major in version. and removed V-Minor Versioning: The change gives an update of minor in version. labels Feb 20, 2023
@ghost ghost merged commit 8be6998 into zonemaster:develop Feb 20, 2023
@ghost ghost mentioned this pull request Feb 20, 2023
@mattias-p
Copy link
Member

I've successfully tested this as part of release testing for 2023.1.

@mattias-p mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 5, 2023
@ghost ghost mentioned this pull request Jun 21, 2023
@ghost ghost deleted the ldns-version branch June 22, 2023 13:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing T-Feature Type: New feature in software or test case description V-Major Versioning: The change gives an update of major in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version of Zonemaster::LDNS
2 participants