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

feat: configure prober in dev #1957

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

roaminggypsy
Copy link
Contributor

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @roaminggypsy)


src/main/k8s/dev/measurement_system_prober.cue line 22 at r2 (raw file):

	_secretName:               _secret_name
	_verboseGrpcClientLogging: true
	_edp1Name:                 _edp1

Sorry, should have caught this in the previous PR. You don't want to support exactly 6 EDPs in the base #MeasurementSystemProber, but rather an arbitrary number. Instead of building _edpResourceNames from the 6 fields, just take in the _edpResourceNames list.

You can still take in individual tags in the dev/local files.


src/main/k8s/dev/BUILD.bazel line 520 at r2 (raw file):

        "image_tag": IMAGE_REPOSITORY_SETTINGS.image_tag,
        "secret_name": SECRET_NAME,
        "edp1_name": SIMULATOR_K8S_SETTINGS.edp1_name,

Just take in two EDP resource names. While our test environments will always have 6, anyone else deploying this only really needs 2 for it to exercise MPC measurements.

Copy link
Contributor Author

@roaminggypsy roaminggypsy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)


src/main/k8s/dev/BUILD.bazel line 520 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Just take in two EDP resource names. While our test environments will always have 6, anyone else deploying this only really needs 2 for it to exercise MPC measurements.

Done.


src/main/k8s/dev/measurement_system_prober.cue line 22 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Sorry, should have caught this in the previous PR. You don't want to support exactly 6 EDPs in the base #MeasurementSystemProber, but rather an arbitrary number. Instead of building _edpResourceNames from the 6 fields, just take in the _edpResourceNames list.

You can still take in individual tags in the dev/local files.

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @roaminggypsy)


src/main/k8s/dev/measurement_system_prober.cue line 22 at r2 (raw file):

You can still take in individual tags in the dev/local files.

I think you misunderstood this part. You still want individual tags for edp1_name and edp2_name in the local and dev configs even if you're passing them in a list to the base definition.

Copy link
Contributor Author

@roaminggypsy roaminggypsy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/k8s/dev/measurement_system_prober.cue line 22 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You can still take in individual tags in the dev/local files.

I think you misunderstood this part. You still want individual tags for edp1_name and edp2_name in the local and dev configs even if you're passing them in a list to the base definition.

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @roaminggypsy)


src/main/k8s/dev/measurement_system_prober.cue line 8 at r4 (raw file):

_edp1:             string @tag("edp1_name")
_edp2:             string @tag("edp2_name")
_edp_resource_names: [_edp1, _edp2]

nit: you can skip this unnecessary intermediate field

Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @roaminggypsy)

@roaminggypsy roaminggypsy force-pushed the roaminggypsy-prober-dev-config branch from aacf3a3 to 8362ce1 Compare December 11, 2024 22:02
@roaminggypsy roaminggypsy changed the base branch from main to renjiez-migrate-aws-duchy December 11, 2024 22:02
Base automatically changed from renjiez-migrate-aws-duchy to main December 11, 2024 22:53
@roaminggypsy roaminggypsy force-pushed the roaminggypsy-prober-dev-config branch from 825f00d to 345771f Compare December 11, 2024 23:45
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @Marco-Premier and @roaminggypsy)


src/main/k8s/dev/BUILD.bazel line 538 at r5 (raw file):

kustomization_dir(
    name = "measurement_system_prober",
    testonly = True,

nit: I don't believe this needs to be testonly. The simulators did due to depending on test secrets directly.


src/main/k8s/dev/BUILD.bazel line 540 at r5 (raw file):

    testonly = True,
    srcs = [
        "resource_requirements.yaml",

You shouldn't need this as it's intended to be deployed in an existing cluster.


src/main/k8s/dev/BUILD.bazel line 546 at r5 (raw file):

    tags = ["manual"],
    deps = [
        "config_files",

Does this actually depend on anything from the config-files ConfigMap, or does it get everything it needs from the certs-and-configs Secret? Do you even mount that ConfigMap in the CUE file?

Copy link
Contributor Author

@roaminggypsy roaminggypsy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @Marco-Premier and @SanjayVas)


src/main/k8s/dev/BUILD.bazel line 540 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You shouldn't need this as it's intended to be deployed in an existing cluster.

Done.


src/main/k8s/dev/BUILD.bazel line 546 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Does this actually depend on anything from the config-files ConfigMap, or does it get everything it needs from the certs-and-configs Secret? Do you even mount that ConfigMap in the CUE file?

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @roaminggypsy)

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.

4 participants