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

adding helm #638

Merged
merged 12 commits into from Sep 4, 2019

Conversation

@karannaoh
Copy link
Member

karannaoh commented Aug 17, 2019

No description provided.

@karannaoh karannaoh requested review from bloodbare and vangheem as code owners Aug 17, 2019
@karannaoh karannaoh added the wip label Aug 17, 2019
@karannaoh karannaoh force-pushed the helm-charts branch from 8f0fcf7 to db1a4a3 Aug 17, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 17, 2019

Codecov Report

Merging #638 into master will increase coverage by 0.64%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
+ Coverage    92.8%   93.45%   +0.64%     
==========================================
  Files         285      285              
  Lines       25298    25298              
==========================================
+ Hits        23478    23641     +163     
+ Misses       1820     1657     -163
Impacted Files Coverage Δ
guillotina/db/storages/pg.py 88.06% <0%> (+0.18%) ⬆️
guillotina/content.py 89.54% <0%> (+0.2%) ⬆️
guillotina/tests/test_postgres.py 100% <0%> (+0.72%) ⬆️
guillotina/db/factory.py 91.75% <0%> (+7.69%) ⬆️
guillotina/tests/fixtures.py 92.36% <0%> (+8.01%) ⬆️
guillotina/db/strategies/dbresolve.py 100% <0%> (+27.77%) ⬆️
guillotina/db/storages/cockroach.py 75.37% <0%> (+48.5%) ⬆️
guillotina/tests/test_cockroach.py 95.58% <0%> (+77.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96fce9b...2af25c0. Read the comment docs.

@karannaoh karannaoh force-pushed the helm-charts branch from 2c28fd1 to 861d9b1 Aug 20, 2019
@karannaoh karannaoh removed the wip label Aug 20, 2019
Copy link
Member

vangheem left a comment

Also make sure that we have an easy way to customize the config.json

Otherwise, make all those settings be in values.yaml

nodePort: {{.Values.exposedPort}}
selector:
run: {{ .Values.name }}
type: NodePort

This comment has been minimized.

Copy link
@vangheem

vangheem Aug 20, 2019

Member

Make this configurable

@karannaoh karannaoh force-pushed the helm-charts branch from ccb91e6 to 9da8ae1 Aug 21, 2019
@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Aug 21, 2019

Also make sure that we have an easy way to customize the config.json

Otherwise, make all those settings be in values.yaml

@vangheem a person can just add a new config.json file according to his need, as we generally do while working. Along with it he as command line options too(which I guess you are asking) by passing an array of argument.

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Aug 21, 2019

@karannaoh I would really love to have a good default config that is customizatable with helm chart values.

Having to provide your own config file completely makes the helm chart much less useful.

@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Aug 21, 2019

@karannaoh I would really love to have a good default config that is customizatable with helm chart values.

Having to provide your own config file completely makes the helm chart much less useful.

@vangheem
https://github.com/plone/guillotina/blob/0a02ce71a40f3408a0c63e42aa06692d734984c6/helm/guillotina/templates/configmap.yaml This is something I tried before(having config.yaml in configmap), I will define all the values inside values.yaml.

@karannaoh karannaoh force-pushed the helm-charts branch from 9da8ae1 to 3a9deb2 Aug 22, 2019
@karannaoh karannaoh requested a review from vangheem Aug 22, 2019
@karannaoh karannaoh force-pushed the helm-charts branch from 6995845 to ce4278b Aug 22, 2019
@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Aug 30, 2019

@vangheem are there more changes you want me to work on?

Copy link
Member

vangheem left a comment

That's exactly what I'm looking for! Just one comment on the naming!

charts/guillotina/values.yaml Outdated Show resolved Hide resolved
@karannaoh karannaoh force-pushed the helm-charts branch from 0d2bc30 to 2af25c0 Aug 31, 2019
@karannaoh karannaoh requested a review from vangheem Aug 31, 2019
@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Sep 2, 2019

@vangheem can we merge it?

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Sep 2, 2019

I think it looks good but didn't we kind of decide that we were going to just work on it here but then the actual chart would go into helm's charts?

@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Sep 2, 2019

I think it looks good but didn't we kind of decide that we were going to just work on it here but then the actual chart would go into helm's charts?

I will create a PR to helm's charts repository. can we have one our own repository in guillotinaweb?

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Sep 4, 2019

@karannaoh hmmm, if we think we want it in both places, maybe we should just merge this then? I'm okay with merging this--just need to know what the best course of action is here.

@karannaoh

This comment has been minimized.

Copy link
Member Author

karannaoh commented Sep 4, 2019

@vangheem I tried to figure out what can be best.. I am seeing most of the projects maintain separate repositories for helm charts.
Our is a small helm chart we can merge it here also.

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Sep 4, 2019

Let's just merge here. It'll be easier to maintain.

@vangheem vangheem merged commit e27e671 into master Sep 4, 2019
5 checks passed
5 checks passed
Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
codecov/patch Coverage not affected when comparing 96fce9b...2af25c0
Details
codecov/project 93.45% (+0.64%) compared to 96fce9b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vangheem vangheem deleted the helm-charts branch Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.