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

Add charts from rancher/charts #79

Merged
merged 2 commits into from
Mar 5, 2021
Merged

Conversation

nickgerace
Copy link
Contributor

@nickgerace nickgerace commented Mar 3, 2021

This PR is split into three commits and may be easily "reviewable" by reviewing commit by commit. It follows the rancher/rancher-operator charts pattern.

  1. Copy chart from rancher/charts
  2. Add helm packaging and prepare the chart for packing

Note: we are not merging the old ResourceSet file yet.

Relevant issues:

Copy link
Member

@jiaqiluo jiaqiluo left a comment

Choose a reason for hiding this comment

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

LGTM

@nickgerace nickgerace requested review from rmweir and removed request for ryansann March 4, 2021 15:51
Copy link

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

Why can't the CRD yaml just bee includes in templates/ with everything else?

charts/rancher-backup/Chart.yaml Show resolved Hide resolved
@nickgerace
Copy link
Contributor Author

nickgerace commented Mar 4, 2021

Why can't the CRD yaml just bee includes in templates/ with everything else?

Responding to @rmweir #79 (review).

For "CRD yaml"

We split the CRDs into their own charts, like rancher/rancher-operator and rancher/fleet.
This is for customizability.

For the CR files

In case you are referring to the ResourceSet CR file, that does exist in templates/.
There is a second ResourceSet file in crds/, but that is a development artifact that is unrelated to the chart.
We are addressing that in a future PR.

README.md Outdated Show resolved Hide resolved
Copy link

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

LGTM

@nickgerace nickgerace requested a review from dramich March 4, 2021 21:48
@dramich dramich requested a review from aiyengar2 March 4, 2021 21:51
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link

@aiyengar2 aiyengar2 left a comment

Choose a reason for hiding this comment

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

@nickgerace, once we add this to this repository, can we tie this to a PR on rancher/charts@dev-v2.5-source to treat Rancher Backup as a forked repository from now on? That way we only make updates in this repository and just update the pointer on rancher/charts, similar to what we do for fleet today.

charts/rancher-backup/Chart.yaml Show resolved Hide resolved
@nickgerace
Copy link
Contributor Author

@nickgerace, once we add this to this repository, can we tie this to a PR on rancher/charts@dev-v2.5-source to treat Rancher Backup as a forked repository from now on? That way we only make updates in this repository and just update the pointer on rancher/charts, similar to what we do for fleet today.

@aiyengar2 already ahead of you ;) rancher/charts#1021

Modify the chart and scripts for packaging. Add quickstart to README and
add DEVELOPING file.
@nickgerace nickgerace merged commit 1ecd1f5 into rancher:master Mar 5, 2021
@nickgerace nickgerace deleted the master-chart branch March 5, 2021 20:25
@nickgerace nickgerace changed the title [DNM 2.5.7] Add charts from rancher/charts [2.5.7] Add charts from rancher/charts Mar 5, 2021
@nickgerace nickgerace changed the title [2.5.7] Add charts from rancher/charts Add charts from rancher/charts Mar 5, 2021
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.

5 participants