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

Add enabledHmssMeasurementConsumers into kingdom config #1688

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

renjiezh
Copy link
Contributor

Also merged two HMSS feature flags.

@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.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @renjiezh)


src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto line 47 at r1 (raw file):

  // List of MeasurementConsumer names who force to enable HMSS protocol
  // regardless the feature flags.
  repeated string enabled_measurement_consumers = 3;

This should be a flag next to --enable-hmss rather than in the ProtocolConfig. That way it's obvious how the two flags interact, or why one might see the HMSS protocol used despite --enable-hmss being false.

Copy link
Contributor Author

@renjiezh renjiezh 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 7 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)


src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto line 47 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should be a flag next to --enable-hmss rather than in the ProtocolConfig. That way it's obvious how the two flags interact, or why one might see the HMSS protocol used despite --enable-hmss being false.

I see. It look clearer. In this case, it is set by command line arguments instead of adding these value in config file.
it will be like:

--enable-hmss=false
--enabled_measurement_consumer measurememtConsumers/123abc1
--enabled_measurement_consumer measurememtConsumers/123abc2

The user needs to updates the k8s yml and redeploy it. Does it work better considering origin's practice?

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 all commit messages.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @renjiezh)


src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto line 47 at r1 (raw file):

Previously, renjiezh wrote…

I see. It look clearer. In this case, it is set by command line arguments instead of adding these value in config file.
it will be like:

--enable-hmss=false
--enabled_measurement_consumer measurememtConsumers/123abc1
--enabled_measurement_consumer measurememtConsumers/123abc2

The user needs to updates the k8s yml and redeploy it. Does it work better considering origin's practice?

You have to update the K8s objects anyway in order to pick up configmap updates, at least if you're using Kustomize the way we recommend. If you're using non-hashed configmaps, then you at least need to restart your deployments.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto line 47 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You have to update the K8s objects anyway in order to pick up configmap updates, at least if you're using Kustomize the way we recommend. If you're using non-hashed configmaps, then you at least need to restart your deployments.

+1 for having this next to the general "enabled" flag. This will be deleted once HMSS is fully launched.

Copy link
Contributor Author

@renjiezh renjiezh 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 7 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)


src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto line 47 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

+1 for having this next to the general "enabled" flag. This will be deleted once HMSS is fully launched.

Done.

Copy link
Member

@kungfucraig kungfucraig 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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

Copy link
Member

@kungfucraig kungfucraig 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: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

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 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

Copy link
Collaborator

@stevenwarejones stevenwarejones 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 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

@renjiezh renjiezh enabled auto-merge (squash) July 18, 2024 17:06
@renjiezh renjiezh merged commit d73baa6 into main Jul 18, 2024
4 checks passed
@renjiezh renjiezh deleted the renjiez-hmss-mc-config branch July 18, 2024 17:46
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Also merged R/F HMSS and Reach HMSS feature flags.
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.

5 participants