-
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 - AMR Template form #166910
Conversation
…rm-template-form
…rm-template-form
…rm-template-form
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
import type { CloudSecurityIntegrationAzureAccountType } from './agent_enrollment_flyout/types'; | ||
|
||
const AzureResourceManagerLink = | ||
'https://azure.microsoft.com/en-us/get-started/azure-portal/resource-manager/?OCID=AIDcmm5edswduu_SEM__k_CjwKCAjwxOymBhAFEiwAnodBLBHx_IB6zMNtNiK3A6rPLhhUPeftXrN3px5mwTO739pWUndPzt27aRoCMKIQAvD_BwE_k_&gad=1'; |
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.
can we remove those additional URL params?
and keep the URL like this:
https://azure.microsoft.com/en-us/get-started/azure-portal/resource-manager
Pinging @elastic/fleet (Team:Fleet) |
@@ -7,21 +7,19 @@ | |||
|
|||
import type { AgentPolicy } from '../types'; | |||
|
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.
Great idea on reusing this function, I think we can export from this file an object with the supported templateUrlFieldName
:
export const SUPPORTED_TEMPLATES = {
CLOUD_FORMATION: 'cloud_formation_template_url',
ARM: 'arm_template_url'
}
this way later we can reuse it like this and avoid typos:
import { getTemplateUrlFromAgentPolicy, SUPPORTED_TEMPLATES } from '../../services';
const cloudFormationTemplateFromAgentPolicy = getTemplateUrlFromAgentPolicy(
SUPPORTED_TEMPLATES.CLOUD_FORMATION,
agentPolicy
);
*/ | ||
export const getCloudFormationTemplateUrlFromAgentPolicy = (selectedPolicy?: AgentPolicy) => { | ||
export const getTemplateUrlFromAgentPolicy = ( | ||
templateUrlFieldName: string, |
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.
we can also export the supported templateUrlFieldName
in this function similar as mentioned in the previous comment
<li> | ||
<FormattedMessage | ||
id="xpack.csp.azureIntegration.armTemplateSetupStep.save" | ||
defaultMessage="Click the Save tand continue button on the bottom right of this page." |
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.
typo here: "Save tand continue"
@@ -62,7 +62,7 @@ describe('getCloudFormationTemplateUrlFromAgentPolicy', () => { | |||
], | |||
}; | |||
// @ts-expect-error | |||
const result = getCloudFormationTemplateUrlFromAgentPolicy(selectedPolicy); | |||
const result = getTemplateUrlFromAgentPolicy(selectedPolicy); |
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.
I believe it's missing one param, also can you add a test for ARM?
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.
Fleet changes LGTM
Thanks @opauloh and @juliaElastic for the reviews. @opauloh all comments were addressed, thanks for the great suggestions :) |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @JordanSh |
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.
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.
Tested Locally LGTM!
We discussed this at private but i'll update in this thread as well. This option is under investigation lead by @orestisfl , we might add it in the future |
Summary
Resolves #165580
Screen.Recording.2023-09-26.at.15.01.17.mov