Skip to content
This repository was archived by the owner on Mar 31, 2023. It is now read-only.

Conversation

@zzxgzgz
Copy link
Contributor

@zzxgzgz zzxgzgz commented Mar 26, 2021

This is a temporary DHCP fix, we need to test it with the cluster.

}

switch (current_DhcpState.operation_type()) {
alcor::schema::OperationType current_operation_type =
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is a temporary fix :), it will be great to add a bunch of comment on why we are doing it, what does it do and when we should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added, thanks for your suggestions 👍

for (int i = 0; i < parsed_struct.port_states().size(); i++) {
ACA_LOG_INFO(
"Port %d MAC: %s, device_ID: %s, device_owner: %s\n", i,
(parsed_struct.port_states().at(i).configuration().mac_address()).c_str(),
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do parsed_struct.port_states(i) instead of parsed_struct.port_states().at(i)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

alcor::schema::OperationType current_operation_type =
current_DhcpState.operation_type();
if (current_operation_type == alcor::schema::OperationType::UPDATE) {
for (auto &port_state : parsed_struct.port_states()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for the goalstatev2, you may direct access the corresponding port_state assuming the port resource ID is the same as the DHCP resource ID. See this for an example:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really a good way to do it, thanks for pointing it out 👍

Copy link
Contributor

@er1cthe0ne er1cthe0ne left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants