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.

Sign up for GitHub

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

Jump to bottom

feat!: make host rule detection configurable and opt-in #12187

Closed
wants to merge 11 commits into from
Closed

feat!: make host rule detection configurable and opt-in #12187

wants to merge 11 commits into from

Conversation

fgreinacher
Copy link
Contributor

@fgreinacher fgreinacher commented Oct 15, 2021 •
edited
Loading

Changes:

Move the logic to detect host rules from environment variables to a separate file. Do not call it during env parsing, but after all configuration has been read so that it can be disabled via all config methods.

Context:

Closes #12155

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

@fgreinacher fgreinacher changed the title refactor: encapsulate hostRulesFromEnv feat: make host rule detection configurable and opt-in Oct 15, 2021
@fgreinacher fgreinacher marked this pull request as ready for review October 15, 2021 22:42
@rarkins rarkins changed the title feat: make host rule detection configurable and opt-in feat!: make host rule detection configurable and opt-in Oct 19, 2021
@rarkins rarkins added the breaking Breaking change, requires major version bump label Oct 19, 2021
@rarkins rarkins marked this pull request as draft October 20, 2021 11:46
@rarkins rarkins marked this pull request as ready for review October 22, 2021 06:15
@rarkins rarkins requested a review from viceice October 22, 2021 06:16
viceice
viceice reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but i think we should split out the refactoring to a separate pr and merge that first.

@viceice
Copy link
Member

viceice commented Oct 22, 2021

Please open pr from private account, otherwise maintainers are not able to update your branches, so it makes it a lot more complicated to merge.

@fgreinacher
Copy link
Contributor Author

LGTM, but i think we should split out the refactoring to a separate pr and merge that first.
Please open pr from private account, otherwise maintainers are not able to update your branches, so it makes it a lot more complicated to merge.

OK, will split to two PRs from my user fork.

This was referenced Oct 22, 2021
Merged
Merged
@fgreinacher
Copy link
Contributor Author

fgreinacher commented Oct 24, 2021 •
edited
Loading

Split into #12277 and #12294.

@fgreinacher fgreinacher deleted the feat/disable-infering-credentials branch October 24, 2021 11:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viceice viceice viceice left review comments

@rarkins rarkins rarkins approved these changes

Assignees
No one assigned
Labels
breaking Breaking change, requires major version bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable inferring credentials from environment variables
3 participants
@fgreinacher @viceice @rarkins