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

Use case-insensitive matching for env variables #1710

Closed
wants to merge 1 commit into from

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Oct 29, 2023

What does this implement/fix?

See title. This PR follows an internal discussion and gives us some version of FTL to play around with. I am still not convinced this is something that should be done, hence, draft pull request for now.

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@DL6ER DL6ER requested a review from PromoFaux October 29, 2023 19:49
@yubiuser
Copy link
Member

I'm not sure we should have this, but I can see how this is preventing CAPS_tYpos. Just a thought: does FTL log a warning if it detects an env variable that starts with FTLCONF_ but could not be mapped to an config key?

@DL6ER
Copy link
Member Author

DL6ER commented Oct 30, 2023

Just a thought: does FTL log a warning if it detects an env variable that starts with FTLCONF_ but could not be mapped to an config key?

No. This needs extra code because we first need to iterate over the entire environment and store any FTLCONF_ in a list. Then we have settings processing marking every parsed variable as used. In the end, we can then check if our list of env vars contains something that has not been marked as processed and issue warnings for those. Definitely worthwhile having I'd say. But I'll first wait what the fate of this PR will be and then either add it here or in a separate PR. Merge conflicts are unavoidable with this PR and another one being open at the same time.

@DL6ER
Copy link
Member Author

DL6ER commented Oct 31, 2023

@yubiuser #1719

@DL6ER
Copy link
Member Author

DL6ER commented Oct 31, 2023

Closing this PR. I don't think we should support case-insensitive variables.

@DL6ER DL6ER closed this Oct 31, 2023
@DL6ER DL6ER deleted the tweak/case_insensitive_env_vars branch October 31, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants