Skip to content

Commit

Permalink
address comments (iter VII)
Browse files Browse the repository at this point in the history
  • Loading branch information
@IbraAoad
IbraAoad committed Aug 26, 2024
1 parent 8cb3437 commit cc58f08
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 133 deletions.
6 changes: 5 additions & 1 deletion .github/workflows/pull-request.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ on:
branches:
- main

# TODO: Remove when #8 closes
# The following configuration is temporarily pinned to this branch to enable Juju to restart hooks,
# which are disabled by default (https://github.com/charmed-kubernetes/actions-operator/blob/5c6377ed695d52b8a1693f07b7d641e245269123/src/bootstrap/index.ts#L205).
# Note: This causes integration tests to fail as charms lose connection to the Juju controller
# when added to the mesh, regaining connection only upon hook retry.
# TODO: Revert to main branch after #8 is resolved.
jobs:
pull-request:
name: PR
Expand Down
8 changes: 4 additions & 4 deletions src/charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,10 @@ def _add_labels(self):
namespace.metadata = ObjectMeta()

# Ensure labels are a dictionary even if they are initially None or not set
if namespace.metadata.labels is None:
namespace.metadata.labels = {}
if namespace.metadata.labels is None: # pyright: ignore
namespace.metadata.labels = {} # pyright: ignore

existing_labels = namespace.metadata.labels
existing_labels = namespace.metadata.labels # pyright: ignore
if (
existing_labels.get("istio.io/use-waypoint")
or existing_labels.get("istio.io/dataplane-mode")
Expand All @@ -209,7 +209,7 @@ def _add_labels(self):
"charms.canonical.com/istio.io.waypoint.managed-by": f"{self._managed_labels}",
}

namespace.metadata.labels.update(labels_to_add)
namespace.metadata.labels.update(labels_to_add) # pyright: ignore
self._patch_namespace(namespace)

def _remove_labels(self):
Expand Down
253 changes: 125 additions & 128 deletions tests/unit/test_charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,166 +21,163 @@ def harness():
harness.cleanup()


def test_add_labels(harness: Harness[IstioBeaconCharm]):
@pytest.mark.parametrize(
"labels_before, patched, labels_after",
[
(
# Assert that, when there are no waypoint labels, the expected labels are added
{},
True,
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
},
),
(
# Assert that existing labels get preserved on positive cases
{"foo": "bar"},
True,
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
),
(
# Assert that, when we already manage the labels, they get updated
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
# "istio.io/dataplane-mode": "ambient", # omitted for this case on purpose
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
True,
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
),
# Assert that, when we we do not manage the labels, they do not get updated
(
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient", # omitted for this case on purpose
"foo": "bar",
},
False,
"unused arg",
),
],
)
def test_add_labels(harness: Harness[IstioBeaconCharm], labels_before, patched, labels_after):
"""Test the _add_labels method with namespace labeling logic."""
harness.begin()
charm = harness.charm

mock_namespace = Namespace(
metadata=ObjectMeta(
name="istio-system",
labels={
"foo": "bar",
},
labels=labels_before,
)
)

# Scenario #1: Namespace has other k8s labels with no istio labels
# Expected: Positive Scenario, istio labels to be updated normally
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:

charm._add_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == {
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
}

# Scenario #2: Namespace labels are managed by another entity
# Expected: Negative Scenario, labels shouldn't be added
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
entity_managed_labels = {
"istio.io/use-waypoint": "another-entity",
"istio.io/dataplane-mode": "ambient",
}
mock_namespace.metadata.labels.update(entity_managed_labels)
charm._add_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_not_called()
assert mock_namespace.metadata.labels == entity_managed_labels

# Scenario #3: Namespace labels are managed by this charm
# Expected: Positive Scenario, istio labels to be updated normally
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
istio_managed_labels = {
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
}
mock_namespace.metadata.labels.update(istio_managed_labels)
charm._add_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == istio_managed_labels

# Scenario #4: Namespace has no labels configured, not even external k8s ones
# Expected: Positive Scenario, istio labels to be updated normally
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
istio_managed_labels = {
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
}
charm._add_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == istio_managed_labels


def test_remove_labels(harness: Harness[IstioBeaconCharm]):
if patched:
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == labels_after
else:
mock_patch.assert_not_called()
assert mock_namespace.metadata.labels == labels_before


@pytest.mark.parametrize(
"labels_before, patched, labels_after",
[
(
# Scenario 1: Namespace labels are managed by this charm
{
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
True,
{
"foo": "bar",
"istio.io/use-waypoint": None,
"istio.io/dataplane-mode": None,
"charms.canonical.com/istio.io.waypoint.managed-by": None,
},
),
(
# Scenario 2: Namespace labels are managed by another entity
{
"istio.io/use-waypoint": "another-entity",
"istio.io/dataplane-mode": "ambient",
"foo": "bar",
},
False,
{
"istio.io/use-waypoint": "another-entity",
"istio.io.dataplane-mode": "ambient",
"foo": "bar",
},
),
(
# Scenario 3: Namespace labels are partially managed by this charm
{
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
True,
{
"foo": "bar",
"istio.io/use-waypoint": None,
"istio.io/dataplane-mode": None,
"charms.canonical.com/istio.io.waypoint.managed-by": None,
},
),
(
# Scenario 4: Namespace has no labels configured at all
{},
False,
{},
),
],
)
def test_remove_labels(harness: Harness[IstioBeaconCharm], labels_before, patched, labels_after):
"""Test the _remove_labels method with namespace labeling logic."""
harness.begin()
charm = harness.charm

mock_namespace = Namespace(
metadata=ObjectMeta(
name="istio-system",
labels={
"istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint",
"istio.io/dataplane-mode": "ambient",
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
},
labels=labels_before,
)
)

# Scenario #1: Namespace labels are managed by this charm
# Expected: Positive Scenario, istio labels to be removed
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:

charm._remove_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == {
"foo": "bar",
"charms.canonical.com/istio.io.waypoint.managed-by": None,
"istio.io/dataplane-mode": None,
"istio.io/use-waypoint": None,
}

# Scenario #2: Namespace labels are managed by another entity
# Expected: Negative Scenario, labels shouldn't be removed
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
entity_managed_labels = {
"istio.io/use-waypoint": "another-entity",
"istio.io/dataplane-mode": "ambient",
}
mock_namespace.metadata.labels.update(entity_managed_labels)
charm._remove_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_not_called()
assert mock_namespace.metadata.labels == entity_managed_labels

# Scenario #3: Namespace has some labels missing, but managed by this charm
# Expected: Positive Scenario, existing istio labels to be removed
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
partial_labels = {
"charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system",
"foo": "bar",
}
mock_namespace.metadata.labels.update(partial_labels)
charm._remove_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == {
"foo": "bar",
"charms.canonical.com/istio.io.waypoint.managed-by": None,
"istio.io/dataplane-mode": None,
"istio.io/use-waypoint": None,
}

# Scenario #4: Namespace has no labels configured at all
# Expected: No operation is performed, as there are no labels to remove
with patch.object(
charm.lightkube_client, "get", return_value=mock_namespace
) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch:
mock_namespace.metadata.labels.clear()
charm._remove_labels()
mock_get.assert_called_once_with(Namespace, "istio-system")
mock_patch.assert_not_called()
assert mock_namespace.metadata.labels == {}
if patched:
mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace)
assert mock_namespace.metadata.labels == labels_after
else:
mock_patch.assert_not_called()
assert mock_namespace.metadata.labels == labels_before


def test_sync_waypoint_resources_add_labels(harness: Harness[IstioBeaconCharm]):
Expand Down

0 comments on commit cc58f08

Please sign in to comment.