Skip to content

feat: Support rbac auth type in conversation_logger #1627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 13, 2025

Conversation

leonavevor
Copy link
Contributor

@leonavevor leonavevor commented Jan 14, 2025

Purpose

  • To support rbac when conversation history is being saved

Does this introduce a breaking change?

  • Yes
  • No

How to Test

  • Run it in devcontainer to test
git clone [[repo-address]](https://github.com/leonavevor/chat-with-your-data-solution-accelerator)
cd chat-with-your-data-solution-accelerator
git checkout main
  • Test the code
code/tests/utilities/helpers/test_azure_search_helper.py => test_get_conversation_logger_rbac()

What to Check

Verify that the following are valid

  • should be able to have your conversations saved even if you are not using key based auth (i.e using RBAC).

Other Information

@leonavevor
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code/backend/batch/utilities/helpers
   azure_search_helper.py59198%269
TOTAL382185877% 

Tests Skipped Failures Errors Time
395 0 💤 0 ❌ 0 🔥 52.293s ⏱️

@leonavevor
Copy link
Contributor Author

leonavevor commented Jun 5, 2025

@Prasanjeet-Microsoft and @Vinay-Microsoft I have committed a better fix for this issue and rebased my recent changes on it, so it is in sync with the upstream main.
This fix is base on AzureSearch library.
This is the new change: 92e7f98

@Prasanjeet-Microsoft
Copy link
Contributor

@Prasanjeet-Microsoft and @Vinay-Microsoft I have committed a better fix for this issue and rebased my recent changes on it, so it is in sync with the upstream main. This fix is base on AzureSearch library. This is the new change: 92e7f98

Hello @leonavevor, Thanks for the updated fix and for incorporating the changes using the AzureSearch library. We will verify the changes and if everything works as expected, we will proceed with merging the PR.

Copy link
Contributor

@Prasanjeet-Microsoft Prasanjeet-Microsoft left a comment

Choose a reason for hiding this comment

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

Approved

@Roopan-Microsoft Roopan-Microsoft added this pull request to the merge queue Jun 13, 2025
Merged via the queue into Azure-Samples:main with commit f1030fa Jun 13, 2025
6 checks passed
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.

3 participants