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

fix(host-rules): support matchHost with a dot prefix #11523

Merged
merged 3 commits into from Sep 2, 2021
Merged

fix(host-rules): support matchHost with a dot prefix #11523

merged 3 commits into from Sep 2, 2021

Conversation

straub
Copy link
Contributor

@straub straub commented Sep 1, 2021

Changes:

host-rules will no longer double up on dot prefixes if the configured value of matchHost
already contains one.

Context:

I found myself wishing the docs called this out when I added my own dot prefix out of what
I thought was an abundance of caution, and inadvertently made it impossible to match any
hostnames due to the one already there :) Hoping to help clarify this for posterity.

Open to suggestions on the wording! I'm not sure I phrased this as clearly as possible or
kept the voice of the docs consistently.

Edit: updated per recommendation below to correct the behavior, in addition to clarifying
in the docs.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@rarkins
Copy link
Collaborator

rarkins commented Sep 1, 2021

Do you mean that we ended up with a "double dot" prefix when you first tried it?

@straub
Copy link
Contributor Author

straub commented Sep 1, 2021

Exactly! I (incorrectly) configured ".docker.io" and matchesHost() wound up trying to do hostname.endsWith('..docker.io') here:

return hostname === rule.matchHost || hostname.endsWith(`.${rule.matchHost}`);

@rarkins
Copy link
Collaborator

rarkins commented Sep 1, 2021

We could/should probably fix that though

@straub
Copy link
Contributor Author

straub commented Sep 1, 2021

Sounds good to me! Something like rule.matchHost.startsWith('.') ? hostname.endsWith(rule.matchHost) : hostname.endsWith(.${rule.matchHost})?

Would we still want to have clarification in the docs that this happens?

Happy to PR this, with guidance on the desired approach. Also thanks for your time as always!

@rarkins
Copy link
Collaborator

rarkins commented Sep 1, 2021

I think that logic change sounds good. You're welcome to expand the explanation in the documentation too at the same time!

@straub straub changed the title docs: clarify matchHost hostname suffix matching fix(host-rules): support matchHost with a dot prefix Sep 2, 2021
@straub
Copy link
Contributor Author

straub commented Sep 2, 2021

@rarkins Updated! Please let me know what you think, at your convenience

@rarkins rarkins enabled auto-merge (squash) September 2, 2021 16:47
@rarkins rarkins merged commit 8fb9197 into renovatebot:main Sep 2, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 26.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants