Skip to content

Conversation

@yaarark
Copy link

@yaarark yaarark commented Jun 3, 2025

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

Copilot AI review requested due to automatic review settings June 3, 2025 09:59
@azure-client-tools-bot-prd
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the functionality of the Application Gateway Firewall custom rule command to support additional variable names.

  • Updated help documentation and the ValidateSet attribute to include "ClientAddrXFFHeader" and "GeoLocationXFFHeader".
  • Revised ChangeLog.md to document the new properties and a corresponding bug fix.
  • Added new scenario tests (PowerShell and C#) to cover the additional custom rule variable functionality.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Network/Network/help/New-AzApplicationGatewayFirewallCustomRuleGroupByVariable.md Updated help text with new accepted values.
src/Network/Network/FirewallPolicy/FirewallCustomRule/GroupByUserSession/GroupByVariable/NewAzureApplicationGatewayFirewallCustomRuleGroupByVariableCommand.cs Extended ValidateSet to include new variable names.
src/Network/Network/ChangeLog.md Added changelog entries documenting the new variables and bug fix.
src/Network/Network.Test/ScenarioTests/ApplicationGatewayTests.ps1 Added PowerShell scenario tests for custom rule variable functionality.
src/Network/Network.Test/ScenarioTests/ApplicationGatewayTests.cs Updated test runner to include new test scripts.

## Upcoming Release

* Added property 'GeoLocationXFFHeader' and 'ClientAddrXFFHeader' as VariableName in `NewAzureApplicationGatewayFirewallCustomRuleGroupByVariable`.
* Fixed bug in `NewAzureApplicationGatewayFirewallCustomRuleGroupByVariable` to add "GeoLocation" as a valid input for VariableName
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

The changelog entry mentions adding 'GeoLocation' as a valid input, but the code and help updates reference 'GeoLocationXFFHeader'. Please update the changelog for consistency with the new accepted values.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the change log to make it consistent with the change.

@vidai-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

## Upcoming Release

* Added property 'GeoLocationXFFHeader' and 'ClientAddrXFFHeader' as VariableName in `NewAzureApplicationGatewayFirewallCustomRuleGroupByVariable`.
* Fixed bug in `NewAzureApplicationGatewayFirewallCustomRuleGroupByVariable` to add "GeoLocation" as a valid input for VariableName
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the change log to make it consistent with the change.

@github-actions
Copy link

github-actions bot commented Jun 3, 2025

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

@vidai-msft
Copy link
Contributor

@yaarark If you add new test cases, please record it from your local and then include the json files in the PR as well.

@YanaXu YanaXu self-assigned this Jun 25, 2025
@YanaXu YanaXu marked this pull request as draft July 1, 2025 02:48
@YanaXu
Copy link
Contributor

YanaXu commented Jul 1, 2025

Hi @yaarark , I've converted this PR to draft. Please update it as ready when your fix the CI errors.

@YanaXu YanaXu removed their assignment Jul 29, 2025
@isra-fel
Copy link
Member

Closing the PR for lack of activity. Feel free to reopen to continue working on it. Thanks 😀

@isra-fel isra-fel closed this Oct 20, 2025
@yoavmal yoavmal reopened this Oct 21, 2025
@yoavmal yoavmal mentioned this pull request Oct 21, 2025
6 tasks
@yoavmal
Copy link
Contributor

yoavmal commented Oct 21, 2025

Closing this PR. Changes moved to this PR.

@yoavmal yoavmal closed this Oct 21, 2025
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.

6 participants