Opened 5 years ago

Closed 5 years ago

#1203 closed change (rejected)

Establish nginx::hostconfig-fragment directive

Reported by: matze Assignee: matze
Priority: Unknown Milestone:
Module: Infrastructure Keywords: nginx, puppet
Cc: trev, fhd Blocked By:
Blocking: Platform: Unknown
Ready: no Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5731052198821888

Description

Background

During #753, setting up the order-system by cloning trac instances, we've applied a mechanism to easily divide nginx(8) host configuration files into fragments. This could become quite handy in other modules as well (see e.g. #240), given that it is established in the nginx::* manifests at modules/nginx/manifests/ themselves (rather than the current setup example in modules/trac/manifests/init.pp).

What to change

  • Establish an nginx::hostconfig-fragment directive performing similar to the aforementioned example
  • Demonstrate it's application by migrating code from #753 (and thus create a proof-of-concept)
  • (*code-review, of course*)
  • Demonstrate it's compatibility with the existing code due to consistency in behavior by performing provisioning tests for each (development) box using the nginx module - given that the review concludes the idea being applicable

Change History (8)

comment:1 Changed 5 years ago by matze

  • Review URL(s) modified (diff)
  • I've created a first draft for establishing the nginx::hostconfig-fragment() directive and published it for review: http://codereview.adblockplus.org/5731052198821888
  • A second patch-set with exemplary changes for the demonstration with the order-system and issues setup will follow in a few minutes
Last edited 5 years ago by matze (previous) (diff)

comment:2 Changed 5 years ago by matze

The 2nd patch set has been published (and smoke-tested). Since the full integration test for all affected boxes is quite time-consuming (for the CPU, not the human), I'll wait for review feedback before continuing - in order to integrate suggestions and improvements before, if any.

comment:3 Changed 5 years ago by matze

There's also a 3rd patch set now; I had to ensure that any fragment modification will trigger nginx(8) to reload.. just a one-liner though.

comment:4 Changed 5 years ago by matze

  • Blocking 240 removed

comment:5 Changed 5 years ago by trev

I am a bit undecided as to whether this is really useful as a generic mechanism. Normally, we don't need fragments for this - we can use templates for the host configuration. So this approach does look like overarchitecturing. @fhd, any opinions?

comment:6 Changed 5 years ago by fhd

I'm not a fan of generalising anything used in only one place. If the same mechanism is going to be used to solve #240 (sounds like it), we should combine this with that change IMO.

comment:7 Changed 5 years ago by matze

Well, although the current setup (regarding the order-system's fragments) feels like a dirty hack in comparison to this approach, I agree that there's no need to integrate this feature unless used somewhere else. In addition, it may not be necessary for #240 but, as @fhd noted already, we should merge the patch set as soon as it's required somewhere else.

Furthermore, in case it may become integrated at some point, one may want to further improve the code so the additional directory (${nginx::fragment_directory}/${domain}) is only created when there's an actual fragment, and not in advance for any host configuration. But this additional effort makes sense only if we plan to integrate it, of course.

comment:8 Changed 5 years ago by trev

  • Resolution set to rejected
  • Status changed from new to closed

Resolving as rejected - while the patch is good enough, it seems that we don't want to use it, at least not until we really need it. At the moment, fragments are only used by the Trac module it this code can stay there.

Note: See TracTickets for help on using tickets.