Set Artifactory version 5.8#3200
Conversation
* Update Artifactory version * Add resources requests and limits to all pods * Ability to set whitelist on Nginx LoadBalancer service * Support setting a custom artifactory service name (that will also be set in nginx config) * Add new parameters to README * Fix artifactory replicas reference. Hardcode database replicas to 1 * Use resource memory requests and limits in Artifactory java options * Add readinessProbe to Artifactory * Add readme on creating a Kubernetes Docker registry secret and passing it to helm
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
size/S
stable/artifactory/values.yaml
Outdated
| # repository: "docker.bintray.io/jfrog/artifactory-oss" | ||
| repository: "docker.bintray.io/jfrog/artifactory-pro" | ||
| version: 5.6.3 | ||
| version: |
There was a problem hiding this comment.
A version should be listed to pin the version for reproducible deployments
There was a problem hiding this comment.
I was actually thinking that having the Chart.yaml's appVersion as a single source of version covers it.
I have a template in the _helpers.tpl called "artifactory.artifactory.version", which is used in the artifactory-deployment.yaml:
image: '{{ .Values.artifactory.image.repository }}:{{ template "artifactory.artifactory.version" . }}'
This way, I don't need to change the version in multiple files.
Did I miss something?
There was a problem hiding this comment.
I didn't see that. I like it.
Prior to merging this I would like to have a quick chat with the other maintainers.
/hold
/assign
There was a problem hiding this comment.
/hold cancel
@eldada we had a conversation about this in the charts chat call today. Several maintainers weighed in. Thank you for being patient with us while we talked this out.
What you've done with the helpers is interesting and on more complicated charts, with multiple images using the same version, could make updates easier. This does make updating the chart easier for maintainers.
But, for those consuming the chart it makes it a little more difficult. What version being used isn't in the README. How you can set it yourself isn't intuitive. I've already witnessed a few people trip over setting their own tag version.
Because we want to prioritize chart consumers and their experience we would prefer to have explicit versions in the values.yaml file instead of pulling a default from the Chart.yaml file. We understand it's a little more work for chart maintainers and there's some duplication. But, it appears to be more intuitive for consumers.
There was a problem hiding this comment.
@mattfarina - I understand. I will revert this to the old format.
I think there should be an effort towards a best practice of a single source for the app version, that is used across all chart. I'll let you and the team play with this idea.
Maybe it's just an issue of better documentation (?)
k8s-ci-robot
added
the
do-not-merge/hold
|
@mattfarina - any estimate on this? Any open issues I should address? |
|
@eldada The meeting where we discuss these sorts of things is on Tuesday of this week. The holidays caused us to go a little longer than normal without meeting. I should have some direction by Wednesday. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@mattfarina - ready for your final review. |
mattfarina
left a comment
mattfarina
left a comment
There was a problem hiding this comment.
Just a couple more nits
stable/artifactory/README.md
Outdated
| | `artifactory.image.pullPolicy` | Container pull policy | `IfNotPresent` | | ||
| | `artifactory.image.repository` | Container image | `docker.bintray.io/jfrog/artifactory-pro` | | ||
| | `artifactory.image.version` | Container image tag | `5.6.3` | | ||
| | `artifactory.image.version` | Artifactory and Nginx containers image tag | | |
There was a problem hiding this comment.
In the docs we'd like to see the version (5.8.2 in this case)
| replicaCount: 1 | ||
| image: | ||
| repository: "docker.bintray.io/jfrog/nginx-artifactory-pro" | ||
| version: 5.6.3 |
There was a problem hiding this comment.
What is someone wanted to override the nginx or artifactory images? They would have change any version tags as a group rather than one or the other.
And, if someone reads the values.yaml file as a consumer of the chart they may be confused on setting the version.
Could you please revert where the versions are combined?
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@eldada I think the github outage had an effect on the CI results reporting. The cla and circleci ones are waiting for a response. Is there any way you could push a new commit that does basically nothing to kick these off. I'm sorry for the inconvenience. Seems this communication between CI tools and GitHub isn't fault tolerant enough. |
|
@mattfarina - What is the "tide" check? What does it mean "Not in tide pool"? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eldada, unguiculus The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
4 participants
Thank you for contributing to kubernetes/charts. Before you submit this PR we'd like to
make sure you are aware of our technical requirements and best practices:
For a quick overview across what we will look at reviewing your PR, please read
our review guidelines:
Following our best practices right from the start will accelerate the review process and
help get your PR merged quicker.
When updates to your PR are requested, please add new commits and do not squash the
history. This will make it easier to identify new changes. The PR will be squashed
anyways when it is merged. Thanks.