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

Move custom resource definitions to separate helm chart #36

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

sgraband
Copy link
Contributor

No description provided.

@jfaltermeier jfaltermeier self-requested a review July 31, 2023 15:12
Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

Thanks, I've verified the approach with the following steps:

  • Removed v7beta dummy change in CRD
  • Used terraform test-configuration to setup minikube and install theia-cloud-base
  • Used helm install -n theiacloud theia-cloud-crds . to install CRDs
  • Continue with terraform test-configuration to install theia-cloud chart
  • Verified Theia Cloud working
  • Readded v7beta dummy change in CRD and updated chart version
  • Used helm upgrade -n theiacloud theia-cloud-crds .
  • CRD upgrade worked. Theia Cloud crashed, as expected, because it does not know about v7beta yet

So this looks really promising.

In order to merge it early, we should

  • update the Chart.yaml's description, version, appVersion
  • Remove the v7beta change
  • Update the terraform configurations in the main repository to install the CRDs via this chart (if you want, I can adjust the terraform configs, just let me know)

@sgraband
Copy link
Contributor Author

Thanks for the fast review. I updated the chart information and removed the v7beta change.

I will also adjust the terraform configurations on the main repository and update the documentation aswell.

@sgraband
Copy link
Contributor Author

Created the main repo PR: eclipse-theia/theia-cloud#213

Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

LGTM, however we should use a prerelease version for the chart version

charts/theia-cloud-crds/Chart.yaml Outdated Show resolved Hide resolved
Co-authored-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>
jfaltermeier added a commit to eclipse-theia/theia-cloud that referenced this pull request Aug 1, 2023
@jfaltermeier jfaltermeier merged commit 5b1b95f into osweek23 Aug 1, 2023
@jfaltermeier jfaltermeier deleted the separateChart branch August 1, 2023 07:25
jfaltermeier pushed a commit to eclipse-theia/theia-cloud that referenced this pull request Aug 1, 2023
jfaltermeier added a commit to eclipse-theia/theia-cloud that referenced this pull request Aug 1, 2023
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.

2 participants