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!: split bind address to manage host and port separately #679

Merged

Conversation

cpitstick-latai
Copy link
Contributor

@cpitstick-latai cpitstick-latai commented Jun 17, 2024

Adds webhook port and bind port for metrics server to the Deployment for the main operator. Also allows customization of the metrics bind port for the operator container. This is to prevent port conflicts when hostNetwork: true is set (#680)

This PR

The bindPort for the metrics server was not customizable for the open-feature-operator. Enabling this to be configured. Also explicitly specify both the metrics server port and the webhook port in the deployment.

Related Issues

Notes

This is necessary for enabling hostNetwork: true which is in this PR

Follow-up Tasks

How to test

On an EKS cluster where the CNI has been replaced with Calico, and combined with hostNetwork: true

@cpitstick-latai cpitstick-latai force-pushed the cpitstick-latai/add-bindport branch 6 times, most recently from 219e6ee to 2f54eab Compare June 17, 2024 21:03
@cpitstick-latai cpitstick-latai force-pushed the cpitstick-latai/add-bindport branch 2 times, most recently from a38d607 to 872d23d Compare June 25, 2024 22:27
@cpitstick-latai cpitstick-latai marked this pull request as ready for review June 25, 2024 22:31
@cpitstick-latai cpitstick-latai requested a review from a team as a code owner June 25, 2024 22:31
@cpitstick-latai cpitstick-latai force-pushed the cpitstick-latai/add-bindport branch 2 times, most recently from caef5a1 to 1d174a5 Compare July 1, 2024 18:48
Allows customization of the metrics bind port for the operator
container as well as the webhook port.

Important for when hostNetwork is `true`

Signed-off-by: Christopher Pitstick <cpitstick@lat.ai>
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.51%. Comparing base (499661e) to head (5e1b1fb).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #679   +/-   ##
=======================================
  Coverage   86.51%   86.51%           
=======================================
  Files          19       19           
  Lines        1587     1587           
=======================================
  Hits         1373     1373           
  Misses        173      173           
  Partials       41       41           
Flag Coverage Δ
unit-tests 86.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@toddbaert
Copy link
Member

My only concern is that things still aren't quite consistent. For example, managerConfig.controllerManagerConfigYaml.health.healthProbeBindAddress includes the : and doesn't work the same as these new params (separate host/port). However, it's the case that things already weren't quite consistent here, and I'm not sure if it's worth addressing in this PR or not.

I'm approving, but I'd like to hear from others. cc @bacherfl @thisthat @odubajDT

@cpitstick-latai
Copy link
Contributor Author

@toddbaert A dev anti-pattern is to hold a PR hostage by demanding that changes that are not in the PR's scope be made to fix other technical debt. I saw this early in my career, and have learned to aggressive fight it.

Fixing the consistency here can be the NEXT PR. But I'm not sure I'm the right person to do this fix, as your team seems to have a particular and peculiar view of what you desire.

Copy link
Contributor

@bacherfl bacherfl left a comment

Choose a reason for hiding this comment

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

lgtm

@beeme1mr beeme1mr changed the title feat: Add bindPort to helm chart feat!: split bind address to manage host and port separately Jul 3, 2024
@toddbaert
Copy link
Member

@toddbaert A dev anti-pattern is to hold a PR hostage by demanding that changes that are not in the PR's scope be made to fix other technical debt. I saw this early in my career, and have learned to aggressive fight it.

Fixing the consistency here can be the NEXT PR. But I'm not sure I'm the right person to do this fix, as your team seems to have a particular and peculiar view of what you desire.

Agreed, we just need 2 approvals and I wanted to point out the implicit choice. I'm fine to merge as is.

@beeme1mr beeme1mr merged commit 31cddba into open-feature:main Jul 3, 2024
17 checks passed
@github-actions github-actions bot mentioned this pull request Jul 3, 2024
@cpitstick-latai cpitstick-latai deleted the cpitstick-latai/add-bindport branch July 3, 2024 17:48
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