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(helm) - Enable configuring service type LoadBalancer with a Static IP #1689

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

mutiadavid
Copy link
Contributor

Enable configuring service type LoadBalancer with a Static IP.

This is very helpful when using cloud providers such as Azure Kubernetes Service (AKS) and you want to assign a static IP to the dragonfly service.

Signed-off-by: David Mutia <davidmutia47@gmail.com>
@Pothulapati
Copy link
Contributor

@mutiadavid Thanks for raising the change and its a valid one. Can you make the following changes so that we can merge this?

  • Rebase the branch
  • And add a test that verifies the behaviour? This is very simple and you can follow the doc and samples

mutiadavid and others added 2 commits August 11, 2023 12:00
Signed-off-by: David Mutia <davidmutia47@gmail.com>
cmdstats_map were on the hotpath and are needed only to update the command stats.
Instead, I introduced the stats withing the CommandId itself that we lookup anyways.
Also, it removes fragile dependency on naked command name char* pointers.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
@mutiadavid
Copy link
Contributor Author

@Pothulapati
I have added the tests.

Please let me know whether the merge from main the repository added is what you meant by rebase.

Thanks.

@romange
Copy link
Collaborator

romange commented Aug 11, 2023

@mutiadavid this PR fails our unit tests.

@romange
Copy link
Collaborator

romange commented Aug 11, 2023

@Pothulapati it's possible that our tests are broken due to recent changes with TLS. You must pass --requirepass to dragonfly when using tls

@mutiadavid
Copy link
Contributor Author

@romange @Pothulapati
The file 'tls-values.golden.yaml' keeps on updating every time I run 'go test -v ./... -update' command. Does it have anything to do with the tests failing?

Changes to that file haven't been committed to the repo.

image

@Pothulapati
Copy link
Contributor

@mutiadavid I'm trying to fix the issue in CI with #1696, Will update you once its merged, for you to rebase.

@Pothulapati
Copy link
Contributor

@mutiadavid Fix just got merged: #1696. Can you rebase your branch?

Thanks!

@mutiadavid
Copy link
Contributor Author

@Pothulapati I've merged the updates from main.

Thanks for fixing the issue.

Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution @mutiadavid

@Pothulapati Pothulapati merged commit 82c3690 into dragonflydb:main Aug 14, 2023
7 checks passed
kostasrim pushed a commit that referenced this pull request Aug 14, 2023
…ic IP (#1689)

* Enable configuring service type LoadBalancer IP

Signed-off-by: David Mutia <davidmutia47@gmail.com>

* Add tests for loadBalancerIP

Signed-off-by: David Mutia <davidmutia47@gmail.com>

* chore: get rid of cmdstats_map (#1687)

cmdstats_map were on the hotpath and are needed only to update the command stats.
Instead, I introduced the stats withing the CommandId itself that we lookup anyways.
Also, it removes fragile dependency on naked command name char* pointers.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

---------

Signed-off-by: David Mutia <davidmutia47@gmail.com>
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
Co-authored-by: Roman Gershman <roman@dragonflydb.io>
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.

None yet

3 participants