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

WIP: Concat experiment #576

Closed
wants to merge 4 commits into from
Closed

WIP: Concat experiment #576

wants to merge 4 commits into from

Conversation

apenney
Copy link
Contributor

@apenney apenney commented Jan 18, 2014

Just trying to see if using concat for the vhost would be an improvement in terms of letting people inject custom content more easily.

I was thinking of having: custom_content => {} and have it take:

{'name' => { 'order' => n, 'content => x }} and then just use create_resources() to spit out the extra fragments in appropriate places. I went with a numbering scheme that hops up by 10 so there's lots of spots to inject custom stuff.

Would this be a good idea, or bad idea, please vote below!

@blkperl
Copy link
Contributor

blkperl commented Jan 19, 2014

I think you want to delete apache/vhost.conf.erb now that you are no longer using it.

@@ -496,6 +418,271 @@
],
notify => Service['httpd'],
}

# nvh_addr_port
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably say "concat uses:" so that contributors are not confused.

@blkperl
Copy link
Contributor

blkperl commented Jan 19, 2014

👍 Change looks good and easier to maintain going forward. Custom_content sounds fine.

@3flex
Copy link

3flex commented Jan 23, 2014

Just curious, if the module introduces support for a new apache module and adds a new fragment, would the priorities of existing fragments change? E.g. if a new concat::fragment is added (priority 260 would be the next one) is there a chance this would clash with custom fragments users added with $custom_content if they set the priority to 260 as well?

I'm not quite sure about Apache's config and whether this is actually an issue or not, and I don't disagree with the approach, but I'm thinking of working on a new module for nginx based on the patterns used in this module so I'm wondering what the best way to deal with that is (and nginx config is certainly affected by the order of location directives at least).

@apenney
Copy link
Contributor Author

apenney commented Jan 23, 2014

It would definitely clash in that case. The main reason I went for round numbers is so that we can document that "all multipliers of 10 are reserved for module purposes." It's kind of clumsy, but it's the best we can do with ordering. I've never actually tested two concat fragments with the same order, it may just deal with them in a random order but in the same overall spot if you do that. I'll have to experiment. :)

@blkperl
Copy link
Contributor

blkperl commented Jan 24, 2014

Just merged, #575 please add a concat fragment and rebase against master.

@igalic
Copy link
Contributor

igalic commented Jan 24, 2014

I'd like to see some docs, either here or in the README.. well, actually, definitely in the README too… on how to use this. We're linking from several other bugs here, but I just realized I just cannot imagine how it's supposed to be used.

@apenney
Copy link
Contributor Author

apenney commented Jan 24, 2014

I'll definitely be writing up a bunch of docs, just waiting on @laurenrother 's big README rewrite/work she's doing. Once that's in I won't clash with it and make her hurt me. :)

@igalic
Copy link
Contributor

igalic commented Jan 24, 2014

Yes, that's all cool and stuff, but please just add a comment or description >= 1 line that actually shows how to use this :O

@igalic
Copy link
Contributor

igalic commented Feb 7, 2014

I think this should be merged in next!

@apenney apenney closed this Mar 27, 2014
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

Successfully merging this pull request may close these issues.

4 participants