Skip to content

Conversation

@ashvindeodhar
Copy link
Member

Current code is incorrectly checking against EndpointPolicyType.

Reason for Change:

Issue Fixed:

Requirements:

Notes:

aegal
aegal previously approved these changes Mar 8, 2021
Copy link
Contributor

@aegal aegal left a comment

Choose a reason for hiding this comment

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

lgtm


Second thought have a question below

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #815 (97c4b50) into master (1e26d07) will increase coverage by 0.43%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
+ Coverage   42.05%   42.49%   +0.43%     
==========================================
  Files         144      144              
  Lines       14051    14198     +147     
==========================================
+ Hits         5909     6033     +124     
- Misses       7427     7434       +7     
- Partials      715      731      +16     

Copy link
Contributor

@aegal aegal left a comment

Choose a reason for hiding this comment

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

Is it valid to rely on Json.unmarshal to detect this is a type of ACL policy, as far as I am aware, if the data defintion is different this doesn't return error as long as you feed it properly formed JSON. So I believe in this case, we will always return ACL policy if it gets there.

@aegal aegal self-requested a review March 8, 2021 18:14
@aegal aegal dismissed their stale review March 8, 2021 18:15

see earlier question

Copy link
Contributor

@aegal aegal left a comment

Choose a reason for hiding this comment

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

lgtm

@ashvindeodhar ashvindeodhar merged commit 9a352f2 into Azure:master Mar 8, 2021
@ashvindeodhar ashvindeodhar deleted the fork-fix-acl-hnsv2 branch March 8, 2021 19:56
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.

2 participants