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

Introduce a docker-compose based testing method #4121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vincentkelleher
Copy link

@vincentkelleher vincentkelleher commented May 16, 2024

Hi 👋

As I've been working on this project recently, I noticed that there wasn't any unified testing method for identifiers and this has been mentioned in issues such as #2258.

This pull request is a proposal for a simple and straightforward testing method based on docker-compose to run the official Apache httpd Docker image. This uses an httpd.conf file that is just the original configuration file with the rewrite engine enabled and AllowOverride All for the main directory.

I've also updated the README to add explanations on how to test identifiers 😊

@dgarijo dgarijo requested a review from davidlehn May 16, 2024 12:26
@dgarijo
Copy link
Collaborator

dgarijo commented May 16, 2024

Cool, this also allows for reproducing the whole deployment.
+1

@vincentkelleher
Copy link
Author

I've pushed a quick update which increases the rewrite_module log level to debug as it's critical for debugging rewrite rules 😇

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
httpd.conf Outdated Show resolved Hide resolved
@vincentkelleher
Copy link
Author

Thank you @TallTed for your suggestions, I've committed them all 👍

@vincentkelleher
Copy link
Author

Hi 👋

Does this PR need some more work before being approved ?

@dgarijo
Copy link
Collaborator

dgarijo commented Jun 10, 2024

@davidlehn any thoughts?

@davidlehn
Copy link
Collaborator

This looks nice. Thanks. I've been a bit busy to test it out properly. Will try to soon.

  • Might be better for httpd.conf to be named something to indicate it's for development? Probably doesn't matter what it's called.
  • Until the PR to move served things to ids/ is merged, I think the .yml and .conf will be served live. Not a big deal or worth addressing.
  • Should the httpd.conf be cleaned up? It's mostly comments. I did want to at least sync the live modules list with what's here so people can test what's actually running.

@vincentkelleher
Copy link
Author

@davidlehn I've just rebased my branch and took into account your first and last comment.

I'm not sure I'm the best person to voice an opinion about your second comment as I don't know how you deploy this project 😇

@vincentkelleher
Copy link
Author

Hi 👋

Can someone review this pull request as it is opened for a while now ? 😇

@egavard
Copy link
Contributor

egavard commented Oct 9, 2024

Hi @davidlehn
Did you have time to take a look at this MR by any chance ?

The changes are quite light and can be used by many to validate their configuration.
Thanks !

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.

5 participants