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

First release of helm chart in new location #1

Merged
merged 16 commits into from
Aug 31, 2020

Conversation

torstenwalter
Copy link
Member

@torstenwalter torstenwalter commented Aug 24, 2020

This is also the first release of the jenkins chart in the new Github org. The README was updated to reflect the new location.

This also adds https://github.com/github/super-linter GitHub action to validate markdown files.
To ensure a nice contributor experience all linting errors have been fixed.

Note: yaml linting is disabled as Helm templates are no valid yaml.

Closes #3

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Copy link
Contributor

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

Otherwise looks good!

Also linking a related follow-up issue for Chart.yaml #7

CODE_OF_CONDUCT.md Show resolved Hide resolved
charts/jenkins/README.md Outdated Show resolved Hide resolved
torstenwalter and others added 6 commits August 25, 2020 18:35
Signed-off-by: Torsten Walter <mail@torstenwalter.de>
… remove when Helm 2 support ends). One sentence per line. Many minor improvements

Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Torsten Walter <mail@torstenwalter.de>
Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Torsten Walter <mail@torstenwalter.de>
README.md Outdated Show resolved Hide resolved
torstenwalter added a commit to torstenwalter/azure that referenced this pull request Aug 26, 2020
As suggested by @timja in jenkinsci/helm-charts#1 (comment)
this adds a CNAME record which points to helm charts

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Signed-off-by: Torsten Walter <mail@torstenwalter.de>
This adds https://github.com/github/super-linter GitHub action to validate markdown files.
To ensure a nice contributor experience all linting errors have been fixed.

Note: yaml linting is disabled as Helm templates are no valid yaml.

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
@torstenwalter torstenwalter changed the title Added README and contributing guidelines First release oh helm chart in new location Aug 27, 2020
scottrigby
scottrigby previously approved these changes Aug 27, 2020
Copy link
Contributor

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

👏

@wmcdona89 wmcdona89 changed the title First release oh helm chart in new location First release of helm chart in new location Aug 28, 2020
@oleg-nenashev
Copy link
Member

CC @jenkinsci/code-reviewers

oleg-nenashev
oleg-nenashev previously approved these changes Aug 29, 2020
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Looks good overall! Note that the "Jenkins master" terminology is now deprecated, it would be great to cleanup the terminology in subsequent pull requests

@@ -5,6 +5,10 @@ numbering uses [semantic versioning](http://semver.org).

NOTE: The change log until version 1.5.7 is auto generated based on git commits. Those include a reference to the git commit to be able to get more details.

## 2.6.0 First release in jenkinsci GitHub org
Copy link
Member

Choose a reason for hiding this comment

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

Should this file get migrated to the repository root? if not, it might make sense to reference it from README

Copy link
Member Author

Choose a reason for hiding this comment

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

It\s now referenced from the chart README.


The following tables list the configurable parameters of the Jenkins chart and their default values.

### Jenkins Master
Copy link
Member

Choose a reason for hiding this comment

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

These tables might have had some value as user documentation for those who have no immediate access to Helm, but I'd guess maintaining them is difficult

Copy link
Member Author

Choose a reason for hiding this comment

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

@wmcdona89 added them back in a separate file.

wmcdona89 and others added 2 commits August 30, 2020 00:17
Signed-off-by: Aaron McDonald <wmcdona89@gmail.com>
docs(jenkins): add VALUES_SUMMARY.md and minor improvements
@scottrigby
Copy link
Contributor

scottrigby commented Aug 31, 2020

Curious why values summary was added back. Generally these are redundant and tend to get out of sync with values.yaml, which hopefully is already readable-ideally more so.

@torstenwalter
Copy link
Member Author

@torstenwalter
Copy link
Member Author

@wmcdona89 I am not really sure if it's a good idea to keep the values table. It's quite some effort to keep it up to date. When removing values or just change the default value of a key one always has to update the table. I think values.yaml should be authorities. We could link to that file in the README instead.

I don't want to explain how to run the image as root.

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
@scottrigby
Copy link
Contributor

scottrigby commented Aug 31, 2020

It's really up to you all as maintainers of the chart. In my PR to your branch I just applied the same template as proposed for the prometheus charts, for the same reasons: 1. simplifying (charts have become a bit overcomplicated, the repeated README table convention is just one way), 2. easier maintenance (and support for when users get frustrated when table values don't work as expected, since they're manually copied for each chart.values change and periodically are wrong).

If you wanted to automate updating the VALUES_SUMMARY.md every time values.yaml is updated, it could address that 🙂

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Signed-off-by: Torsten Walter <mail@torstenwalter.de>
@torstenwalter
Copy link
Member Author

I added some more changes:

  • top-level README now at least contains a reference to the Jenkins chart
  • migration steps from one version to another is moved into a migration guide section at the bottom. I think it's something people are only interested once, while the more common use case is to look up configuration examples.
  • moved the JCasC configuration to the top of the configuration section as this one is an enabler for users as it allows to make the majority of changes.

I also noticed that we still have XML configuration examples in the README. These should be removed or replaces with JCasC alternatives in future PRs.

@torstenwalter
Copy link
Member Author

I had another look in the file describing the values. What I really like about it is the grouping of related values together. Therefor I am ok with leaving them for now.

Generating it from values.yaml will not provide us the same experience. Using new keys for values to do the grouping will be a breaking change. What we could try is to sort them in the value.yaml the same way and use comments for seperation. Downside is that keys are not sorted alphabetically.

@torstenwalter
Copy link
Member Author

So this PR is up for a last review round. We should try to move future improvements to new PRs to unblock the migration and allow people to contribute.

@wmcdona89
Copy link
Contributor

wmcdona89 commented Aug 31, 2020

I opened issue #18 to organize values.yaml to align with the sections in VALUES_SUMMARY. then the README can link to each section in values.yaml and VALUES_SUMMARY can be removed.

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.

release first version of the chart in the new location
6 participants