Skip to content

Conversation

@ecigar13
Copy link
Contributor

@ecigar13 ecigar13 commented Jun 1, 2022

Reason for Change:

Issue Fixed:

Requirements:

Notes:

@rbtr
Copy link
Collaborator

rbtr commented Jun 3, 2022

can you update the title to be descriptive of the change? it becomes the commit message on merge so it is important that it provides immediately useful information.

@ecigar13 ecigar13 changed the title Style/14519104 remove unneeded errors, fix error message and return early if CNI state exist Jun 3, 2022
@ecigar13 ecigar13 changed the title remove unneeded errors, fix error message and return early if CNI state exist return early if CNI state exist and add some style fixes Jun 3, 2022
@rbtr rbtr enabled auto-merge (squash) June 6, 2022 15:40
@rbtr rbtr disabled auto-merge June 6, 2022 15:40
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

lgtm

return
}

if cniStateExists {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: with this we can remove the below !cniStateExists on line 115

@ecigar13 ecigar13 merged commit 08b07d8 into Azure:master Jun 6, 2022
@ecigar13 ecigar13 deleted the style/14519104 branch June 6, 2022 15:52
}

if cniStateExists {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should put a comment here explaining why we return if cniStateExists. Even for folks that have seen this code path before, it's not immediately obvious.

matmerr pushed a commit to matmerr/azure-container-networking that referenced this pull request Jun 29, 2022
* style: format issues

* style: lint issues

* style: lint issues

* style: moved to new block

Co-authored-by: Keith Nguyen <keithnguyen@microsoft.com>
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