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

Jump to bottom

ncm-hostsfile: Cleanup and tests #1502

Merged
merged 7 commits into from Jun 28, 2021
Merged

ncm-hostsfile: Cleanup and tests #1502

merged 7 commits into from Jun 28, 2021

Conversation

jrha
Copy link
Member

@jrha jrha commented Apr 14, 2021

Add test for working IPv4 only behaviour.

ned21
ned21 reviewed Apr 20, 2021
View reviewed changes
ncm-hostsfile/src/main/perl/hostsfile.pm Outdated Show resolved Hide resolved
ncm-hostsfile/src/main/perl/hostsfile.pm Outdated Show resolved Hide resolved
@jrha
Copy link
Member Author

jrha commented Apr 20, 2021

I could do with some more examples of how this component is used in the wild to build more test cases, right now it looks to me like the behaviour of takeover doesn't match the documentation.

@ned21
Copy link
Contributor

ned21 commented Apr 23, 2021 •
edited

Looks like we use takeover=true. What sort of examples would you like? Looks like our default profile is:

+-/software/components/hostsfile
   +-entries
      +-<<fqdn>>
         $ ipaddr : "<<ipaddr>>"
      +-localhost
         $ ipaddr : '127.0.0.1'
   $ takeover : true

@stdweird
Copy link
Member

test this please

@jrha jrha modified the milestones: 21.8, 21.6 Apr 29, 2021
ned21
ned21 requested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@ned21 ned21 left a comment

Choose a reason for hiding this comment

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

Can you add a test for takeover=true using the sample profile I provided above please?

I've added backwards incompatible because this removes short name support, but looking at our /etc/hosts file, I believe we are still using it. Can you restore that functionality please?

ncm-hostsfile/src/main/pan/components/hostsfile/schema.pan Outdated Show resolved Hide resolved
ncm-hostsfile/src/main/pan/components/hostsfile/schema.pan Outdated Show resolved Hide resolved
@jrha
Copy link
Member Author

jrha commented Jun 28, 2021

Can you add a test for takeover=true using the sample profile I provided above please?

I've added backwards incompatible because this removes short name support, but looking at our /etc/hosts file, I believe we are still using it. Can you restore that functionality please?

No problem, sample profile added and functionality restored.

@jrha jrha force-pushed the hostsfile_tests branch 5 times, most recently from ab25bf9 to fbf39d7 Compare June 28, 2021 11:11
@jrha
Copy link
Member Author

jrha commented Jun 28, 2021

I think leaving the backwards incompatible flag in place is justified as the schema now has some type checking in place.

@jrha jrha merged commit 7686b8e into quattor:master Jun 28, 2021
@jrha jrha deleted the hostsfile_tests branch June 28, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stdweird stdweird stdweird left review comments

@ned21 ned21 ned21 approved these changes

Assignees
No one assigned
Labels
backwards incompatible
Milestone
21.12
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jrha @ned21 @stdweird