Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764
base: main
Are you sure you want to change the base?
Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764
Changes from 30 commits
1702f14
09f9d04
4119ef8
9ed666f
da93eee
0264589
22af527
f9abbb7
5fc3ec2
2b18667
8a08eb2
d6c54fe
54ad50a
02bd29c
16e7169
e3680d2
8c41f6d
10ec3e8
e6ce450
b6d0848
91c85c7
8d03c7f
efd3c2e
8f292ec
b7750b2
e711ea4
69ae41a
a1bf072
41f99e6
2df6e9a
1d2a760
09e7ac7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, we are no longer checking anywhere that the variables are not empty.
I suggest we don't add the 4 new booleans at all, and instead we rely on the existence of account name or not (for example).
Example pseudo-code:
You could even go one step further and make sure the old verification code is cleaned up automatically after the obsoletion period has passed
Example pseudo-code:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other overload of SetMicrosoftManagedAuthorization is now unused after the function in the other codeunit is removed.
So you need to wrap the old overload of SetMicrosoftManagedAuthorization into
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no entry in verification log, then the remaining grace period should be 0.
It means that azure account was never verified and hence they are not entitled to grace period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are doing StrSubstNo inside LogTelemetry already. It seems like we don't need it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataClassification::CustomerContent
The account name is customer content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to wrap this procedure (including docs comments) within preprocessor symbols:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make sure our automations remove the code after the obsoletion period has passed. It's also the root cause of the failure you see in the automated tests for this PR.
Check failure on line 104 in src/System Application/App/AI/src/Azure OpenAI/AzureOpenAI.Codeunit.al
GitHub Actions / Build System Application and Tools (Clean) / System Application and Tools (Clean)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are obsoleting a function in the facade codeunit, you are leaving some code in this codeunit unused after the obsoletion period has passed.
In particular the old overload of
SetManagedResourceAuthorization
will not be called by anyone anymore and it should hence be removed after the obsoletion period has passed.This codeunit is marked with
access=internal
, which means you don't need to explicitly obsolete the function with an[Obsolete()]
tag because noone can reference this code outside of its own AL extension.But you still need to make sure we don't leave unused code in the repos after the automations clean up the obsoleted code.
The way to do it here is to wrap the old overload of
SetManagedResourceAuthorization
in the tags:This way, when we remove the code from the other codeunit because it's wrapped in CLEAN tags (see my other comment), this will also be removed (our automations will just remove whatever is inside the CLEAN tags, there is no smartness there, so it's up to the developer to decide what to put inside these tags).