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

Add args to nginx redirect map. #129 #130

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

villanlu
Copy link

No description provided.

@Schnitzel
Copy link
Contributor

so after looking at this closer, it seems that this will break this:

# ~^example\.com\/test\/(.*) http://example.net/$1$is_args$args;

basically the (.*) would capture the args as well and then inject them as $1 again, but then $is_args$args would add the args again, so we would have double args that would break the redirect :/

so I think this PR would break Backwards-Compatibility.
Maybe we should do it this way:

    map $host$uri $redirectdomain {
        include /etc/nginx/redirects-map.conf;
    }
    map $host$uri$args $redirectdomain {
        include /etc/nginx/redirects-map-args.conf;
    }

so that people that want to use this, can add their redirects with args to /etc/nginx/redirects-map-args.conf ?

@villanlu
Copy link
Author

@Schnitzel that solution would definitely work for us too, having 2 separate maps

@tobybellwood
Copy link
Member

Having the separate args-map would require it to be included into the repo and copied into the image as per
https://github.com/uselagoon/lagoon-images/blob/main/images/nginx/Dockerfile#L38

Can you duplicate https://github.com/uselagoon/lagoon-images/blob/main/images/nginx/redirects-map.conf with a couple of examples?

@villanlu
Copy link
Author

@tobybellwood all done - map hierarchy @ nginx.conf might need some more testing

@Schnitzel Schnitzel self-requested a review March 25, 2021 23:33
@villanlu
Copy link
Author

helper still needed

@tobybellwood tobybellwood added this to the 21.04 milestone Mar 25, 2021
@tobybellwood tobybellwood modified the milestones: 21.4.0, 21.5.0 May 6, 2021
@seanhamlin
Copy link
Contributor

seanhamlin commented May 7, 2021

If I understand this PR correctly, this is about enabling Nginx to have a map file built in that would allow redirecting based not only on host and URI, but query parameters as well.

99% of the redirects I do in Nginx are not based on query params, but I can see the possibility where this would be useful I guess (e.g. legacy site migration that uses query params to Lagoon).

What I would like to see

  • clearer explanations in the redirects-map-args.conf file with when you would use this file over the other map file redirects-map.conf.

Questions

  • Is args the best terminology here? I almost always refer to these as query parameters. Perhaps the map file would be best named redirects-with-query-parameters-map.conf or something?

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