Skip to content

Conversation

@saisujithreddym
Copy link
Contributor

@saisujithreddym saisujithreddym commented Jan 30, 2020

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Per comment, I think thsi is best addressed by parameter sets. Also, needs a changelog update

else
{

if (FirewallPolicyId != null && (this.ApplicationRuleCollection != null || this.NetworkRuleCollection != null || this.NatRuleCollection != null))
Copy link
Member

Choose a reason for hiding this comment

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

This seems like something that should be enforced through parameter sets - FirewallPolicyId should not be in the same parameter sets that include ApplicationRUleCollection or NetworkRuleCollection

Copy link
Contributor Author

@saisujithreddym saisujithreddym Jan 31, 2020

Choose a reason for hiding this comment

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

I am having trouble with the mutual exclusion scenario @markcowl placing firewall policy id in one parameter set and rule collections in another did not achieve the desired result.

I have tried various combinations but could not get it done. Do you have an example where we can achieve mutual exclusion using paramter sets? I will keep exploring as well

Copy link
Contributor Author

@saisujithreddym saisujithreddym Feb 3, 2020

Choose a reason for hiding this comment

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

We have decided to fail the call from NRP when there are both the policy and the rule collections. This way we need not make changes on the PS when we try to support policy+collections in case, show a message which is common across multiple platforms and track the usage. Also I would like to make a PR against network january branch and not the master. I have created another PR: #11013 am closing this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants