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

Fixes #19104 - ignore tagged includes for ISC DHCP #517

Closed
wants to merge 1 commit into from

Conversation

lzap
Copy link
Member

@lzap lzap commented Mar 31, 2017

Our parser limitations is a real pain, I stubmbled this upon at customer again. This is a simple idea to allow at least workarounding this by including files from a directory with "igored-by-foreman" element. All these files will not be included at all, so the workaround is then:

subnet XYZ {
    valid_syntax;
    include "/etc/dhcpd/ignored-by-foreman/invalid_subnet_xyz.conf"
}

@lzap
Copy link
Member Author

lzap commented Mar 31, 2017

@witlessbird "I have a new implementation of isc config parser that will parse any valid configuration file. I'll see if I can finish it in the upcoming sprint."

I see, but if you are not against this, I would like to get this merged so I can backport it in Satellite 6.2 and 6.3. We are unlikely be able to backport whole parser rewrite there and customers are hitting this hard. I will create new ticket.

@lzap lzap changed the title Fixes #17800 - ignore tagged includes for ISC DHCP Fixes #19104 - ignore tagged includes for ISC DHCP Mar 31, 2017
@dmitri-d
Copy link
Member

I don't feel comfortable merging this change. Any chance you can make this change downstream-only, as a quick/temporary fix for the customer?

@lzap
Copy link
Member Author

lzap commented Mar 31, 2017

Sure there is, but what about upstream users? To be honest, since this is small enough change I'd actually would like to propose for 1.15 branch backport. Do you see some possible issues with this patch? If I understand correctly, you plan to completely rewrite this part, it will be dropped in the future.

@dmitri-d
Copy link
Member

I understand your rationale for this fix, but I don't think it belongs in upstream. My issues with this change are:

  • it's a workaround, not a fix
  • It will require changes in the docs, that will need to be changed back after it is removed
  • Users will still come with the issues in the parser (and probably again when this workaround is removed)
  • I'm not sure how our puppet modules will handle such a change

@lzap
Copy link
Member Author

lzap commented Mar 31, 2017

It is a pragmatic way to allow our users to actually use ISC DHCP with our crappy parser, I am not presenting this as the definitive solution but a temporary workaround enabler. Sometimes we need to do reasonable steps to get things working, proper parser is not anywhere near and we are suffering from this quite long time now.

I can do docs, that's not big deal

When users will come with the issues, we can actually tell them how to do something. Current state is status quo - we have several issues in the tracker and that's pretty much about it. No answer, no solution.

Our puppet modules do not work with any changes in dhcp configuration files today, this change does not make this worse.

@dmitri-d
Copy link
Member

I agree that current status-quo isn't great; parser-related issues come up with a certain frequency. My guess it's an issue every two-three months? There are no recently opened parser issues that haven't been resolved either. I don't think the amount of pressure we are under (if any) in regards to our parser issues warrants such a fix.

Like I said above, I don't feel comfortable merging this change. You will need to find another maintainer who would be ok with merging it if you insist in having it in the core.

@lzap lzap closed this Apr 3, 2017
@lzap lzap deleted the include-skip-17800 branch May 23, 2017 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants