Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

added loadBalancerIP option to service - Kibana #666

Conversation

alexgordonpandera
Copy link

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py

Opened to address feature #611

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 16, 2020

💚 CLA has been signed

@alexgordonpandera
Copy link
Author

I have now signed the CLA

Copy link
Member

@jmlrt jmlrt 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 you need to merge master in your branch as there is some already implemented changes in your PR.

Also adding a description in the README like in https://github.com/elastic/helm-charts/blame/master/elasticsearch/README.md#L179 could be great (we should add description for other service.XXXsubvalues for better documentation but that we can do it later in another PR).

kibana/templates/service.yaml Show resolved Hide resolved
kibana/tests/kibana_test.py Show resolved Hide resolved
@jmlrt
Copy link
Member

jmlrt commented Jun 17, 2020

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 17, 2020

cla/check

…bana

added loadBalancerIP option to service - Kibana
@alexgordonpandera
Copy link
Author

As for the README. You do not have the same structure in your README for Kibana. It does not makes sense to copy what was in elasticsearch unless you are going to add all of the services.xxx sub-values to the README. Currently it links to the values.yaml, which has been updated to display how to use loadBalancerIP.

I notice it failed the linter in the CI/CD pipeline. What errors came from the linter so I can fix them.

@jmlrt
Copy link
Member

jmlrt commented Jun 17, 2020

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 18, 2020

I notice it failed the linter in the CI/CD pipeline. What errors came from the linter so I can fix them.

https://devops-ci.elastic.co/job/elastic+helm-charts+pull-request+lint-python/285/label=docker&&virtual&&ubuntu-18.04/consoleText:

+ make lint-python
black --diff --check --exclude='ve/|venv/' .
--- kibana/tests/kibana_test.py	2020-06-17 21:15:10.466160 +0000
would reformat kibana/tests/kibana_test.py
+++ kibana/tests/kibana_test.py	2020-06-17 21:16:00.210955 +0000
@@ -465,10 +465,11 @@
 
     r = helm_template(config)
 
     assert r["service"][name]["spec"]["ports"][0]["nodePort"] == 30001
 
+
 def test_adding_a_loadBalancerIP():
     config = ""
 
     r = helm_template(config)
 
@@ -480,10 +481,11 @@
     """
Oh no! 💥 💔 💥
1 file would be reformatted, 8 files would be left unchanged.
 
     r = helm_template(config)
 
     assert r["service"][name]["spec"]["loadBalancerIP"] == "12.4.19.81"
+
 
 def test_override_the_serverHost():
     config = """
     serverHost: "localhost"
     """
make: *** [lint-python] Error 1

=> You should have 2 white lines between each function.

https://devops-ci.elastic.co/job/elastic+helm-charts+pull-request+template-testing/664/CHART=kibana,label=docker&&virtual/consoleText:

helm lint --strict ./
==> Linting ./
Error: 1 chart(s) linted, 1 chart(s) failed
[ERROR] templates/: render error in "kibana/templates/service.yaml": template: kibana/templates/service.yaml:6:13: executing "kibana/templates/service.yaml" at <include "kibana.labels" .>: error calling include: template: no template "kibana.labels" associated with template "gotpl"

make: *** [../helpers/common.mk:27: lint] Error 1

I guess that's not directly related to the loadBalancerIP change but to the other changes that appears in your commit.

@jmlrt
Copy link
Member

jmlrt commented Jun 18, 2020

Your commit still contains some changes which are not relevant for this PR and were already applied in other PR:

Could reset your branch and reapply your changes related to loadBalancerIP:

# reset branch
git reset origin/master --hard

# apply your changes to kibana/templates/service.yaml kibana/tests/kibana_test.py kibana/values.yaml
# ...

# recreate commit
git add kibana/templates/service.yaml kibana/tests/kibana_test.py kibana/values.yaml
git commit -m 'added loadBalancerIP option to service - Kibana '

# push force
git push --force

@alexgordonpandera alexgordonpandera force-pushed the feature/loadBalancerIpKibana branch from 7af7e11 to 47b4202 Compare June 18, 2020 14:07
@alexgordonpandera
Copy link
Author

Alright it should be good to go

@jmlrt
Copy link
Member

jmlrt commented Jun 25, 2020

That's really strange, your new commit 444a2cc still contain the changes from other PRs.

I'm sorry for this but maybe it would be easier to recreate a new branch from master and a new PR to have only the changes require.

I don't see another solution.

@debojitkakoti
Copy link
Contributor

Is it merged in different PR?

@jmlrt
Copy link
Member

jmlrt commented Jul 16, 2020

Replaced by #726

@jmlrt jmlrt closed this Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants