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

Improvements to the helm chart #522

Merged
merged 2 commits into from
Feb 12, 2018
Merged

Improvements to the helm chart #522

merged 2 commits into from
Feb 12, 2018

Conversation

vdboor
Copy link
Contributor

@vdboor vdboor commented Feb 9, 2018

With this PR I'd like to fix the hurdles I've run into.

These are:

  • one bug: the source: postgresURI line was merged the the comment above, because the if statement strips newlines at both ends.
  • the memory limits are too low, causing Clair to be scheduled on hosts that doesn't have enough memory available - and eventually OOM the container or machine.
  • Switch to the stable release, as the git version doesn't work out of the box.
  • Explicitly mention that postgresql.enabled can be set to false, because the requirements.yaml allows that.

Do you also have plans to move this chart to the official helm chart repositories?

The configuration was merged with the comment line above due to usage of
the {{- ...  -}} construct.
Copy link
Contributor

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

Just one minor nitpick.
Thanks a bunch for giving this some love.

tag: latest
pullPolicy: Always
repository: quay.io/coreos/clair
tag: v2.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this code is in master, it should be tied to deploying master. When we cut a stable release, we should tie this to a particular version.

- be more accurate in memory requests, so chair is scheduled on a host
  that has that space available.
- add an explicit mention for the postgresql.enabled flag
@vdboor
Copy link
Contributor Author

vdboor commented Feb 12, 2018

Thanks for looking into this. I've updated my pull request to omit that image change!

I do think it might be useful to mention this choice in the docs, so people can choose to install the stable version instead. I didn't get the master version running last week.

Copy link
Contributor

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

Agreed. Can you file an issue?

@jzelinskie jzelinskie merged commit 5e7b450 into quay:master Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants