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

feat: use regex for ignoring hosts #779

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silviumarcu
Copy link

Allow to use regexes for ignore_hosts to easily ignore hosts that are frequently changed e.g. pipelinesghubeus*.actions.githubusercontent.com

Allow to use regexes for `ignore_hosts` to easily ignore hosts that are frequently changed e.g. `pipelinesghubeus*.actions.githubusercontent.com`
@hartwork
Copy link
Collaborator

hartwork commented Dec 8, 2023

I would like to note that this change would break API backwards-compatibility (e.g. because . no longer matches just literal dots) and hence would need a major version bump if merged.

@silviumarcu
Copy link
Author

@hartwork do you have any suggestion on how to modify it to make it API backwards compatible?

@jairhenrique
Copy link
Collaborator

The signature of ignore_hosts could be like this:

ignore_hosts: set[str | re.Pattern]

In the _build_ignore_hosts you can check the type of each item of set and do the validation for strings and regex.

@silviumarcu
Copy link
Author

silviumarcu commented Jan 15, 2024

Correct me if I am wrong, but that will work only if the regex pattern is compiled, re.compile("regex"). If it's passed as a normal string r"regex" then it's just a plain string when checking the type so it will use the non-regex implementation.

@vEpiphyte
Copy link

vEpiphyte commented Jan 15, 2024

Even if the function signature is changed, that still has an interaction with the ignore_localhost option which adds multiple items to the (potentially empty) hosts_to_ignore set. If the handling of that that option was changed to just add its own filter function, then the function _build_ignore_hosts could change to the following:

    @staticmethod
    def _build_ignore_hosts(hosts_to_ignore):

        if isinstance(re.Pattern):
            def filter_ignored_hosts(request):
                # DISCUSS: Would need to decide if search() or match() is best here.
                # search() tends to be more user friendly behavior
                # match() is more specific though.
                if hasattr(request, "host") and hosts_to_ignore.search(request.host):
                    return
                return request

        else:
            hosts_to_ignore = set(hosts_to_ignore)
            def filter_ignored_hosts(request):
                if hasattr(request, "host") and request.host in hosts_to_ignore:
                    return
                return request

        return filter_ignored_hosts

Then in the config.py code:

        if ignore_localhost:
            filter_functions.append(self._build_ignore_hosts(("localhost", "0.0.0.0", "127.0.0.1")))
        if ignore_hosts:
            filter_functions.append(self._build_ignore_hosts(ignore_hosts))

This should be backwards compatible, at the cost of splitting out the localhost filter function from the existing host filters.

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