Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Rename lifecycle-sidecar to consul-sidecar #810

Merged
merged 1 commit into from
Feb 9, 2021
Merged

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Feb 8, 2021

Changes proposed in this PR:

  • deprecates helm value lifecycleSidecarContainer and replaces it with
    consulSidecarContainer.
  • replaces all uses of lifecycle sidecar
    with consul sidecar, including container names, commands, and flags.

This is to enable future functionality within the consul-sidecar
container, since it may no longer be only responsible for managing
lifecycle.

How I've tested this PR:
acceptance tests against consul-k8s rename-sidecar branch

How I expect reviewers to test this PR:
code review

Checklist:

  • Bats tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@@ -155,7 +155,7 @@ jobs:
-kubecontext="kind-dc1" \
-secondary-kubecontext="kind-dc2" \
-debug-directory="$TEST_RESULTS/debug" \
-consul-k8s-image=docker.mirror.hashicorp.services/hashicorpdev/consul-k8s:latest
-consul-k8s-image=docker.mirror.hashicorp.services/hashicorpdev/consul-k8s@sha256:d158c0b4956421889e7ba80d82c24baa365c5cd3148dac62b555b4c9ba800c0a
then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be removed before merging. This is to use the docker image built for the corresponding consul-k8s change when running acceptance tests.

@ndhanushkodi ndhanushkodi marked this pull request as ready for review February 8, 2021 05:13
@@ -155,7 +155,7 @@ jobs:
-kubecontext="kind-dc1" \
Copy link
Contributor Author

@ndhanushkodi ndhanushkodi Feb 8, 2021

Choose a reason for hiding this comment

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

Does it make sense to change references to lifecycle-sidecar in hack/helm-reference-gen to also use consul-sidecar? I figure the exact values are getting stale anyway and that the use is meant to test that nested nodes work, etc.

Choose a reason for hiding this comment

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

Yup!! That sounds right. It would be nice if the fixture looked even lesser like our values file because it definitely is confusing. I don't think it needs to be updated and can be left as is.

@ndhanushkodi ndhanushkodi requested review from a team and ishustava and removed request for a team February 8, 2021 05:17
Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This looks great!! Excellent work on the pair of PRs @ndhanushkodi

@@ -155,7 +155,7 @@ jobs:
-kubecontext="kind-dc1" \

Choose a reason for hiding this comment

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

Yup!! That sounds right. It would be nice if the fixture looked even lesser like our values file because it definitely is confusing. I don't think it needs to be updated and can be left as is.

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great!

This commit deprecates helm value lifecycleSidecarContainer and replaces it with
consulSidecarContainer. It also replaces all uses of lifecycle sidecar
with consul sidecar, including container names, commands, and flags.

This is to enable future functionality within the consul-sidecar
container, since it may no longer be only responsible for managing
lifecycle.
@ndhanushkodi ndhanushkodi merged commit 3505db3 into master Feb 9, 2021
@ndhanushkodi ndhanushkodi deleted the rename-sidecar branch February 9, 2021 04:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants