Skip to content

Conversation

@ZetaoZhuang
Copy link
Contributor

@ZetaoZhuang ZetaoZhuang commented Oct 20, 2023

Reason for Change:

fix: skip setting SdnRemoteArpMacAddress when hns is not enabled
Issue Fixed:

Requirements:

Notes:

@ZetaoZhuang ZetaoZhuang requested review from a team as code owners October 20, 2023 08:16
@ZetaoZhuang ZetaoZhuang force-pushed the fix_set_sdn_remote_arp branch from 9323adc to 6b5cc8d Compare October 20, 2023 08:18
@ZetaoZhuang ZetaoZhuang force-pushed the fix_set_sdn_remote_arp branch from 6b5cc8d to d85d77d Compare October 20, 2023 18:17
"-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State -Name SDNRemoteArpMacAddress -Value \"12-34-56-78-9a-bc\""

// Command to check if system have hns path or not
CheckIfHNSPathExistsCommand = "Test-Path " +
Copy link
Member

Choose a reason for hiding this comment

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

Please add HNSState in the var for correctness - CheckIfHNSStatePathExistsCommand

func SetSdnRemoteArpMacAddress(execClient ExecClient) error {
exists, err := execClient.ExecutePowershellCommand(CheckIfHNSPathExistsCommand)
if err != nil {
errMsg := fmt.Sprintf("Failed to check the existent of hns path due to error %s", err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

nit; HNS State path

mockExecClient := NewMockExecClient(false)
// testing skip setting SdnRemoteArpMacAddress when hns not enabled
mockExecClient.SetPowershellCommandResponder(func(_ string) (string, error) {
return "false", nil
Copy link
Member

Choose a reason for hiding this comment

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

nit: return False and True instead of false / true to reflect the actual values returned ( which are case sensitive which will also test the equalFold )

Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
@ZetaoZhuang ZetaoZhuang enabled auto-merge (squash) October 27, 2023 22:50
Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
Copy link
Contributor

@msvik msvik left a comment

Choose a reason for hiding this comment

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

looks good overall

Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
Signed-off-by: ZetaoZhuang <106119232+ZetaoZhuang@users.noreply.github.com>
@ZetaoZhuang ZetaoZhuang merged commit aab8e71 into master Nov 1, 2023
@ZetaoZhuang ZetaoZhuang deleted the fix_set_sdn_remote_arp branch November 1, 2023 05:13
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