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

add bindaddress option #110

Merged
merged 3 commits into from
Jul 6, 2021
Merged

add bindaddress option #110

merged 3 commits into from
Jul 6, 2021

Conversation

jhunt-steds
Copy link
Contributor

Pull Request (PR) description

Add the bindaddress configuration option to chrony.conf.

This will allow users to specify an IP address on an interface on which chronyd should listen.

This Pull Request (PR) fixes the following issues

n/a

@jhunt-steds
Copy link
Contributor Author

jhunt-steds commented Mar 30, 2021

I realize the integration tests are failing for CentOS. I believe they are actually failing currently on your existing master branch as well (despite the passing badge); when I made a trivial change to README.md in my fork and had it run the test suite, it failed with the exact same errors this branch does. You can see these results in the details linked from https://github.com/jhunt-steds/puppet-chrony/pull/2/checks .

In case this is useful information, I ran the tests locally as well, using the instructions for Beaker and Docker at https://github.com/voxpupuli/puppet-chrony/blob/679302f6ff7f13a196e26c23492a08259158922c/.github/CONTRIBUTING.md#unit-tests-in-docker . I ran into the issue that the --pidfile-workaround option is presumably meant to solve (with systemd complaining it would not start chronyd because of a symlink issue with the pidfile) when I used the latest CentOS 7 container, but the tests all passed when I used a 7.6 container.

If I'm way off on this or I've done something incorrectly I beg your patience; this is the first time I've done something like this. I would be happy to provide any more information I can or make any needed changes. Thanks!

@aboe76 aboe76 requested review from ekohl and alexjfisher March 30, 2021 19:46
@jasonknudsen jasonknudsen mentioned this pull request Jul 1, 2021
@jasonknudsen
Copy link
Contributor

+1

manifests/init.pp Outdated Show resolved Hide resolved
@jhunt-steds
Copy link
Contributor Author

I've updated my PR to use Stdlib::IP::Address per the comments above; the spec tests (bundle exec rake test) pass locally.

@smortex smortex self-requested a review July 1, 2021 22:16
Copy link
Member

@smortex smortex 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!

@smortex smortex added the enhancement New feature or request label Jul 2, 2021
@smortex
Copy link
Member

smortex commented Jul 3, 2021

@jhunt-steds CI should be fixed in master. Can you please rebase on top of it and force push to update this PR?

@jhunt-steds
Copy link
Contributor Author

@smortex I hope I've done the right thing here--I don't have much experience with the rebase subcommand.

@smortex
Copy link
Member

smortex commented Jul 6, 2021

Weird, your commits appear twice and other commits are listed by GitHub… meh…

Let's fix this :-)

  1. Ensure master is up-to-date on your machine:
git checkout master
git pull
git checkout add-bindaddress-config-opt
  1. Rebase interactively (-i):
git rebase -i master

This will show up your editor with all commits between master and your current branch, I see all your commits twice:

pick c93e335 add bindaddress option
pick 863c97a change bindaddress type to Stdlib::IP::Address
pick 2168ec3 convert bindaddress to an array
pick 4b09bf5 add bindaddress option
pick 11ef42c change bindaddress type to Stdlib::IP::Address
pick ad414f2 convert bindaddress to an array

Just remove the first 3 lines or the last 3 lines, save and quit. Git will remove these commits from history and the rebase should succeed! If so, then proceed: if you git push -f this PR should only show your 3 commits 🎉

@jhunt-steds
Copy link
Contributor Author

I followed your instructions exactly. Thanks for the step-by-step guide!

@smortex smortex merged commit 4ab1f96 into voxpupuli:master Jul 6, 2021
@smortex
Copy link
Member

smortex commented Jul 6, 2021

Thanks!

@jhunt-steds
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants