Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Remove cassandra chart #258

Merged
merged 5 commits into from
Feb 27, 2018

Conversation

kragniz
Copy link
Contributor

@kragniz kragniz commented Feb 26, 2018

We should encourage users to directly use the CassandraCluster and ElasticsearchCluster resources. The helm chart creates a layer of abstraction I don't think we need, and there is no corresponding elasticsearch chart.

This instead uses the same templating elasticsearch uses for e2e testing (envsubst).

/cc @mattbates

Remove cassandra chart

@kragniz kragniz force-pushed the remove-cassandra-chart branch 8 times, most recently from 4dd1fe4 to 7de21ef Compare February 26, 2018 17:56
@kragniz kragniz changed the title WIP: Remove cassandra chart Remove cassandra chart Feb 26, 2018
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @kragniz

Looks good me, but:

  • add a couple of notes to the PR description explaining the motivation for this change (summary of our conversation yesterday)
  • I think maybe give @mattbates a heads up incase he's planning on using the helm chart in demos.
  • Create a followup issue for your idea of syncing the testdata manifests with those in the docs.
  • Make the cluster name a parameter too (looks like that was what you intended anyway.)

apiVersion: "navigator.jetstack.io/v1alpha1"
kind: "CassandraCluster"
metadata:
name: "test"
Copy link
Member

Choose a reason for hiding this comment

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

This should be a CASS_NAME parameter I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@kragniz kragniz force-pushed the remove-cassandra-chart branch 2 times, most recently from cb6dc6f to a87d125 Compare February 27, 2018 17:16
@kragniz
Copy link
Contributor Author

kragniz commented Feb 27, 2018

Now rebased on master with tests passing again

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jetstack-ci-bot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@kragniz
Copy link
Contributor Author

kragniz commented Feb 27, 2018

/retest

@jetstack-ci-bot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@jetstack-ci-bot
Copy link
Contributor

Automatic merge from submit-queue.

@jetstack-ci-bot jetstack-ci-bot merged commit 434172b into jetstack:master Feb 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants