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.

Sign up for GitHub

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

Jump to bottom

Limit old profile_example.json to only properties not used in default profile and rename it #1120

Merged
merged 6 commits into from
Sep 21, 2022
Merged

Limit old profile_example.json to only properties not used in default profile and rename it #1120

merged 6 commits into from
Sep 21, 2022

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Aug 3, 2022 •
edited
Loading

Purpose

Today profile_example.json is a superset of profile.json, or almost. Every time profile.json is updated profile_example.json should also be updated, but that does not always happen. To resolve that problem this PR makes profile_example.json limited to the properties that are not present in the default profile, and it is renamed to better reflect the new content.

Profiles.md is also updated.

The only file in Zonemaster-Engine that reference to profile_example.json is Profiles.md. No other repository has any reference to the file.

Context

#1118

How to test this PR

Documentation only.

@matsduf matsduf added the A-Documentation Area: Documentation only. label Aug 3, 2022
@matsduf matsduf added this to the v2022.2 milestone Aug 3, 2022
@matsduf matsduf mentioned this pull request Aug 4, 2022
Merged
tgreenx
tgreenx reviewed Aug 4, 2022
View reviewed changes
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me. But I will wait for other reviews before approving.

mattias-p
mattias-p previously approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@ghost
Copy link

ghost commented Aug 19, 2022

I think I like having a single file that lists all available properties. It feels odd to me to have a second file with only extra parameters. But I understand that having 2 files is harder to maintain.

@matsduf
Copy link
Contributor Author

matsduf commented Aug 19, 2022 •
edited
Loading

I think I like having a single file that lists all available properties. It feels odd to me to have a second file with only extra parameters. But I understand that having 2 files is harder to maintain.

@PNAX, I agree that there are pros and cons, but my conclusion is to resolve #1118 with this PR. Do you have another proposal how to resolve the issue?

@ghost
Copy link

ghost commented Aug 22, 2022

I don't have any better idea for now. Maybe just a suggestion on renaming profile_example.json into profile_extra.json (or something similar since it's not a viable example anymore).

@matsduf
Copy link
Contributor Author

matsduf commented Aug 23, 2022

I don't have any better idea for now. Maybe just a suggestion on renaming profile_example.json into profile_extra.json (or something similar since it's not a viable example anymore).

I have no problem with that, and I think you have a valid point. I will update this PR with an updated file name.

@matsduf matsduf force-pushed the limit-profile_example.json branch from 078814d to 5904b19 Compare August 23, 2022 20:27
@matsduf matsduf requested review from tgreenx and mattias-p August 23, 2022 20:27
@matsduf
Copy link
Contributor Author

matsduf commented Aug 23, 2022

@PNAX, please review.

ghost
ghost previously approved these changes Aug 24, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beside the typo, this looks good to me

docs/Profiles.md Outdated
[Profile properties]: https://metacpan.org/pod/Zonemaster::Engine::Profile#PROFILE-PROPERTIES
The content of the two files, as-is or modified, can be merged into a custom
profile file that can be loaded by Zonemaster-Engine. Both Zonemaster-CLI and
Zonemaster-Backend has direct options for loading a custom profile file.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "has" -> "have"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected.

@matsduf matsduf changed the title Limit profile_example.json to only properties not used in default profile Limit old profile_example.json to only properties not used in default profile and rename it Aug 25, 2022
@matsduf matsduf requested a review from a user August 25, 2022 02:51
@matsduf
Copy link
Contributor Author

matsduf commented Aug 25, 2022

@mattias-p, are you also fine with the new name? Please re-review.

tgreenx
tgreenx previously approved these changes Aug 25, 2022
View reviewed changes
mattias-p
mattias-p previously approved these changes Sep 16, 2022
View reviewed changes
Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matsduf
Updates Profiles.md
faa38b4 
* Updates to reflect the name change of share/profile_example.json to
  share/profile_additional_properties.json.
* Adds further clarifications on custom profile file.
@matsduf
Corrects typo from review comment
9cb721c
@matsduf matsduf dismissed stale reviews from mattias-p and tgreenx via 9cb721c September 19, 2022 05:11
@matsduf matsduf force-pushed the limit-profile_example.json branch from 74e1136 to 9cb721c Compare September 19, 2022 05:11
@matsduf
Copy link
Contributor Author

matsduf commented Sep 19, 2022

Conflict resolved by rebasing.

@matsduf
Copy link
Contributor Author

matsduf commented Sep 19, 2022

@tgreenx and @mattias-p, can you re-approve? The PR has been rebased to resolve a conflict.

@matsduf matsduf merged commit 6ce577d into zonemaster:develop Sep 21, 2022
@matsduf matsduf deleted the limit-profile_example.json branch September 21, 2022 13:10
@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Dec 13, 2022
@matsduf matsduf mentioned this pull request Dec 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgreenx tgreenx tgreenx approved these changes

@mattias-p mattias-p mattias-p approved these changes

@hannaeko hannaeko Awaiting requested review from hannaeko

@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

Assignees
No one assigned
Labels
A-Documentation Area: Documentation only. V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Milestone
v2022.2
Development

Successfully merging this pull request may close these issues.
3 participants
@matsduf @mattias-p @tgreenx