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

Rewrite of dhcpd-formula #26

Open
mthibaut opened this issue Feb 18, 2018 · 5 comments
Open

Rewrite of dhcpd-formula #26

mthibaut opened this issue Feb 18, 2018 · 5 comments

Comments

@mthibaut
Copy link
Contributor

mthibaut commented Feb 18, 2018

Hi,

I am wondering what you will accept. I don't want my work to go to waste... If I do the work, will you accept it?
Also: shall I put my "new" stuff under a new pillar like the nginx formula, i.e. like this:

dhcpd:
    ng:
        authoritative: true

This would stop any name clashing between the old and the new stanzas. But it would make things awkward, perhaps, since IPv6 isn't even present in the old style.

Any thoughts?

@javierbertoli
Copy link
Member

Hi @mthibaut, first thing first, I'd say your work will be welcome, I don't see a reason to consider it a wasted effort. I think that a (major) rewrite will mean a lot of consideration before being approved and merged. But I'd say it will be definitely considered.

Now, going to the work you're doing, I think that merging tests (#25) would be the first thing to do (I'll try to do it between today and tomorrow), and the move on with the refactoring.

On that issue, I personally prefer:

  • a refactoring that does not imply a ng, but rather modify/improve the formula as is.
  • that the refactoring consider, as much as possible, backward compatibility, at least in states and pillar formats
  • if possible, you can consider submitting different PRs, with smaller changes, (ie, just adding IPv6 support) so they can be evaluated and merged quicker: always consider that most of us do this on our spare/free time, and therefore reviewing big PRs is definitly going to take more time.

Just my 2c.

@mthibaut
Copy link
Contributor Author

Thanks for your comments!

a refactoring that does not imply a ng, but rather modify/improve the formula as is.

As you can see in the ntp formula and in PR #18 there is no way to fix the issues in dhcpd-formula except by starting over from scratch. We would end up with really weird configs containing an impossible-to-understand mish mash of underscores and dashes. Putting things inside "ng" is the best of both worlds, allowing people to use the old syntax while providing a more complete and functional "new" syntax.

that the refactoring consider, as much as possible, backward compatibility, at least in states and pillar formats

The current refactoring has the same top level structure, it just introduces new macros and templates to fill in the blanks. If we want to fix some of the issues in dhcpd-formula that can be fixed, we will automatically converge slowly anyway -- with the only exception being these macros and templates. The convergence would be mainly for OS families and the like. IPv6 as well I think.

if possible, you can consider submitting different PRs, with smaller changes, (ie, just adding IPv6 support) so they can be evaluated and merged quicker: always consider that most of us do this on our spare/free time, and therefore reviewing big PRs is definitly going to take more time.

I do understand, I have the same issue :). I can give it a go for IPv6 though this introduces a batch of new statements. But for the refactoring this will be difficult; there I would rather see an agreement on the approach or a suggestion of a different approach. Would you want some examples of why we need the stuff in PR #18?

@IliyanIliev1
Copy link

Guys, does anybody from you knows how should be written pillar to add option between class {}

@baby-gnu
Copy link
Contributor

Hello.

I just created the pull request #31 which add travic CI and enable semantic-release.

When it will be merged, I'll start to convert the existing formula to template-formula standard without changing how it works (not like #18).

Regards.

@myii
Copy link
Member

myii commented Jul 31, 2019

@mthibaut Another benefit of implementing semantic-release is that it allows for major rewrites, as we've already done with some of the ng formulas. So if you're still willing to make a large contribution, we'd be better placed to accommodate for it after that. Of course, it would be good if the plans can be agreed first between the major contributors, to ensure no-one wastes any time.

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

No branches or pull requests

5 participants