-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cloud Security] Azure integration manual fields #171069
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
2f4c8e0
init added fields, waiting for integration pr to be merged
JordanSh 7c8290f
add all fields, waiting for integration pr to be merged
JordanSh 537963a
before adding field clearing
JordanSh a25c4a8
added tests, need to add field clearing and finalize titles and infos
JordanSh 2b20d42
Merge branch 'main' of https://github.com/elastic/kibana into azure-m…
JordanSh b12d7c7
added field clearing
JordanSh 16f79d5
added version rendering of new ui
JordanSh 1eb7b37
added test
JordanSh 46732af
clean
JordanSh fac3fc8
fixed default value
JordanSh f5adfaf
fixed default value
JordanSh a3b3b64
removed unneeded test
JordanSh 05fab2a
Merge branch 'main' into azure-manual-fields
kibanamachine 3acdeaa
Merge branch 'main' into azure-manual-fields
jeniawhite File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
nit: I find it a bit confusing that when "package is valid for manual" we return
managed identity
otherwisemanual
. without the context, I would think it should be the other way around. Maybe change toisPackageVersionValidForManagedIdentity
?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.
umm this is a very good point, thing is that "manual fields" are meant to reference the entire options (fields) under the Manual credential type, managed identity is just one of them and it happens to be the default value out of those options.
do you think something like
isPackageVersionValidForManualOption
is better? i dont mind sticking with your suggestions, just wanted to give more context and see what do you thinkThere 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.
yeah, I see where the confusion is coming from, that we have Manual as a term that includes multiple setup methods, and inside it we have another
Manual
as its own setup method, as in "really manual, you need to do everything yourself" kind of thing. changing toisPackageVersionValidForManualOption
tbh wouldn't change much for me, in the end starting from 1.7.0 we started to supportManual -> Managed Identity
so it became the default, and before it wasManual -> Manual
the default one. SoisPackageVersionValidForManagedIdentity
still makes more sense to me (even though it's not only Managed Identity but some other options we started to support) but I'm also ok with a comment in this function to explain this Manual extravaganza, without changing the name :)