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

chore: Add config defaults #65

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
chore: Add config defaults
We have a lot of region_name repitition, this sets defaults that match our configuration so that we can just remove those lines from the configuration.yaml entirely.

Also fixes test_lint failing.
rgibert committed Nov 29, 2024
commit c95eee6c8fd1111b408ee13a73ef8dcb592eb3c2
48 changes: 35 additions & 13 deletions libsentrykube/config.py
Original file line number Diff line number Diff line change
@@ -67,14 +67,32 @@ class K8sConfig:
materialized_manifests: str

@classmethod
def from_conf(cls, conf: Mapping[str, Any]) -> K8sConfig:
def from_conf(cls, region_name: str, conf: Mapping[str, Any] | None) -> K8sConfig:
root = str(conf["root"]) if conf is not None and "root" in conf else "k8s"

cluster_def_root = (
str(conf["cluster_def_root"])
if conf is not None and "cluster_def_root" in conf
else f"clusters/{region_name}"
)

cluster_name = (
str(conf.get("cluster_name"))
if conf is not None and "cluster_name" in conf
else None
)

materialized_manifests = (
str(conf["materialized_manifests"])
if conf is not None and "materialized_manifests" in conf
else f"materialized_manifests/{region_name}"
)

Copy link
Member

Choose a reason for hiding this comment

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

There are a 4 instances of checks of conf is not None here. Wonder if it would be better to do the conf is None check once and set the required fields in that scenario. Then the other scenarios don't need that check and the code would be easier to comprehend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya I was on the fence of the clarity of this vs setting it twice.

return K8sConfig(
root=str(conf["root"]),
cluster_def_root=str(conf["cluster_def_root"]),
cluster_name=str(conf.get("cluster_name"))
if "cluster_name" in conf
else None,
materialized_manifests=str(conf["materialized_manifests"]),
root=root,
cluster_def_root=cluster_def_root,
cluster_name=cluster_name,
materialized_manifests=materialized_manifests,
)


@@ -88,18 +106,22 @@ class SiloRegion:

@classmethod
def from_conf(
cls, silo_regions_conf: Mapping[str, Any], sites: Mapping[str, Site]
cls,
region_name: str,
silo_regions_conf: Mapping[str, Any],
sites: Mapping[str, Site],
) -> SiloRegion:
name_from_conf = silo_regions_conf.get("sentry_region", region_name)
bastion_config = silo_regions_conf["bastion"]
assert (
bastion_config["site"] in sites
), f"Undefined site {bastion_config['site']}"
k8s_config = silo_regions_conf["k8s"]
k8s_config = silo_regions_conf["k8s"] if "k8s" in silo_regions_conf else None
return SiloRegion(
bastion_spawner_endpoint=bastion_config["spawner_endpoint"],
bastion_site=sites[bastion_config["site"]],
k8s_config=K8sConfig.from_conf(k8s_config),
sentry_region=silo_regions_conf.get("sentry_region", "unknown"),
k8s_config=K8sConfig.from_conf(name_from_conf, k8s_config),
sentry_region=name_from_conf,
service_monitors=silo_regions_conf.get("service_monitors", {}),
)

@@ -123,8 +145,8 @@ def __init__(self) -> None:
"silo_regions" in configuration
), "silo_regions entry not present in the config"
silo_regions = {
name: SiloRegion.from_conf(conf, sites)
for name, conf in configuration["silo_regions"].items()
region_name: SiloRegion.from_conf(region_name, region_conf, sites)
for region_name, region_conf in configuration["silo_regions"].items()
}

self.sites: Mapping[str, Site] = sites
11 changes: 11 additions & 0 deletions libsentrykube/tests/config.yaml
Original file line number Diff line number Diff line change
@@ -35,3 +35,14 @@ silo_regions:
cluster_def_root: clusters/my_other_customer
materialized_manifests: rendered_services
sentry_region: st-my_other_customer
region2:
bastion:
spawner_endpoint: https://localhost:12345
site: saas_us
k8s:
root: k8s
sentry_region: region2
region3:
bastion:
spawner_endpoint: https://localhost:12345
site: saas_us
36 changes: 36 additions & 0 deletions libsentrykube/tests/test_config.py
Original file line number Diff line number Diff line change
@@ -73,4 +73,40 @@ def test_config_load() -> None:
sentry_region="st-my_other_customer",
service_monitors=MappingProxyType({}),
),
"region2": SiloRegion(
bastion_spawner_endpoint="https://localhost:12345",
bastion_site=Site(
name="us",
region="us-central1",
zone="b",
network="global/networks/sentry",
subnetwork="regions/us-central1/subnetworks/sentry-default",
),
k8s_config=K8sConfig(
root="k8s",
cluster_def_root="clusters/region2",
cluster_name=None,
materialized_manifests="materialized_manifests/region2",
),
sentry_region="region2",
service_monitors=MappingProxyType({}),
),
"region3": SiloRegion(
bastion_spawner_endpoint="https://localhost:12345",
bastion_site=Site(
name="us",
region="us-central1",
zone="b",
network="global/networks/sentry",
subnetwork="regions/us-central1/subnetworks/sentry-default",
),
k8s_config=K8sConfig(
root="k8s",
cluster_def_root="clusters/region3",
cluster_name=None,
materialized_manifests="materialized_manifests/region3",
),
sentry_region="region3",
service_monitors=MappingProxyType({}),
),
}
4 changes: 1 addition & 3 deletions libsentrykube/tests/test_lint.py
Original file line number Diff line number Diff line change
@@ -106,16 +106,14 @@ def test_lint() -> None:
}

errors = kube_linter(MANIFEST, inclusions=enabled_checks)
assert len(errors) == 8
assert len(errors) == 6

assert [check["Check"] for check in errors] == [
"latest-tag",
"no-anti-affinity",
"no-read-only-root-fs",
"run-as-non-root",
"unset-cpu-requirements",
"unset-cpu-requirements",
"unset-memory-requirements",
"unset-memory-requirements",
]