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

Document the process of updating a rke2-chart #1607

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

manuelbuil
Copy link
Contributor

Signed-off-by: Manuel Buil [email protected]

Proposed Changes

This PR explains the process of updating a helm chart so that the change is reflected in both rke2 and rancher

Types of Changes

This PR only updates documentation

@manuelbuil manuelbuil requested a review from a team as a code owner August 11, 2021 10:20
@manuelbuil manuelbuil force-pushed the workflow branch 2 times, most recently from 97ed4e5 to f78c668 Compare August 11, 2021 13:58
Copy link
Contributor

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Good job, no objections to the content, only 2 nits

@@ -0,0 +1,89 @@
# Updating RKE2 charts

In each release, it is very common to update the charts that rke2 is using. This document describes the process that must be followed to succeed in updating and integrating it into rke2 and Rancher.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please limit the line length to some reasonable amount - 80 or 100? It seems like it's done in all the other .md files and IMO it's way better for text editors (easier to find some particular word in a particular line, easier to jump over with vim shortcuts, looks nicer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What examples have you seen? By looking other md files they are also exceeding the 80-100 characters, e.g. https://raw.githubusercontent.com/rancher/rke2/master/developer-docs/upgrading_kubernetes.md. In any case, I can see how this is better, let me try to change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the new version. It sucks that I'll need to create one extra line just for the links because they are pretty long. Or is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the example of other md file:

https://raw.githubusercontent.com/rancher/rke2/master/BUILDING.md

The limit seems to be 120. So maybe let's stick to 120?

I'm fine with links exceedeing the limit. Or in general, I don't mind few exceptions of a bit longer lines, as long as they're an exception rather than a rule. :) But I'm against putting whole paragraphs in single lines.


From the previous section, you know the name of the chart (e.g. `rke2-calico`) and its version (e.g. `v3.19.2-201`). Using that information, we can easily update rke2 and Rancher following the next steps:

1 - Open [Dockerfile](https://github.com/rancher/rke2/blob/master/Dockerfile) and go to the stage where the charts are build (`FROM build AS charts`). There, modify the line where we are referring to the chart, e.g. for calico:
Copy link
Contributor

@vadorovsky vadorovsky Aug 12, 2021

Choose a reason for hiding this comment

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

nit: Github will probably show it nicer (with indentation and numbers as points) if you use 1. instead of 1 -. I might be wrong though, feel free to disregard if that's the case. ;)

@manuelbuil manuelbuil force-pushed the workflow branch 4 times, most recently from 0b63ab8 to 88a7dff Compare August 12, 2021 14:45
Copy link
Contributor

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

@manuelbuil manuelbuil merged commit 6396cef into rancher:master Aug 12, 2021
@manuelbuil manuelbuil deleted the workflow branch October 7, 2021 14:29
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.

4 participants