-
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
[Uptime] [Synthetics Integration] browser - add script recorder option #115184
[Uptime] [Synthetics Integration] browser - add script recorder option #115184
Conversation
…ptime-synthetics-integration-browser-add-script-recorder-option
…browser-add-script-recorder-option
…browser-add-script-recorder-option
…browser-add-script-recorder-option
…cript-recorder-option' of https://github.com/dominiqueclarke/kibana into feature/366-uptime-synthetics-integration-browser-add-script-recorder-option
Pinging @elastic/uptime (Team:uptime) |
5ed2368
to
e2ccf3a
Compare
…ptime-synthetics-integration-browser-add-script-recorder-option
@elasticmachine merge upstream |
…browser-add-script-recorder-option
} | ||
}; | ||
|
||
// @ts-expect-error update 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.
I don't remember why I added this. I'll remove it because I don't think it's necessary. If it is, I'll add a comment.
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're referring to the @ts-expect-error
?
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.
Had a few comments but overall looks great. Let's get this merged ASAP.
const [showScript, setShowScript] = useState(false); | ||
const { isEditable } = usePolicyConfigContext(); | ||
|
||
const handleUpload = useCallback( |
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.
Why not call the hook at the parent level?
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.
By parent level, you mean in the SourceFields
component where ScriptRecorderFields
is rendered?
I suppose the reason I have it here is separation of concerns/domain. Uploading is only relevant to the ScriptRecorderFields
, not the rest of the options in SourceFields
, like zip url or inline.
Does that answer your question? I'm uncertain if I understood fully.
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.
Sure that's fine, just wanted to make sure we had a reason for not simply wrapping the callback where it's declared vs. after it's been passed as a prop. Looking at the callback we declare to pass here, I don't see any problem with leaving it as it is.
<EuiFlyout | ||
ownFocus | ||
onClose={() => setShowScript(false)} | ||
aria-labelledby="syntheticsBrowerScriptBlockHeader" |
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.
👍
); | ||
} | ||
|
||
const MOCK_FILE_NAME = i18n.translate( |
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.
const MOCK_FILE_NAME = i18n.translate( | |
const PLACEHOLDER_FILE_NAME = i18n.translate( |
For clarity's sake can we rename this to avoid overloading the term MOCK
?
|
||
export function Uploader({ onUpload }: Props) { | ||
const fileReader = useRef<null | FileReader>(null); | ||
const [error, setError] = useState<string | null>(''); |
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.
const [error, setError] = useState<string | null>(''); | |
const [error, setError] = useState<string | null>(null); |
Any reason why not?
const parsedContent = `${content}`; | ||
|
||
if (content?.trim().slice(0, 4) !== 'step') { | ||
throw new Error('inline scripts must begin with the keyword "step"'); |
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.
throw new Error('inline scripts must begin with the keyword "step"'); | |
throw new Error('Inline scripts must begin with the keyword "step"'); |
const parsedContent = `${content}`; | ||
|
||
if (content?.trim().slice(0, 4) !== 'step') { | ||
throw new Error('inline scripts must begin with the keyword "step"'); |
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 we're only handling this error I think we don't need to throw it in the first place. Would be faster to use a conditional block, if I'm understanding this ok.
} | ||
}; | ||
|
||
// @ts-expect-error update 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.
You're referring to the @ts-expect-error
?
}; | ||
|
||
return ( | ||
<> |
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.
<> |
Remove.
…ptime-synthetics-integration-browser-add-script-recorder-option
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.
Checked that the TLS protocol issue was fixed, and feedback implementation seems fine.
LGTM WFG.
@elasticmachine merge upstream |
…browser-add-script-recorder-option
x-pack/plugins/uptime/public/components/fleet_package/browser/source_field.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/uptime/public/components/fleet_package/browser/source_field.tsx
Outdated
Show resolved
Hide resolved
All looks good to me. Ship it. |
…cript-recorder-option' of https://github.com/dominiqueclarke/kibana into feature/366-uptime-synthetics-integration-browser-add-script-recorder-option
@elasticmachine merge upstream |
…browser-add-script-recorder-option
@elasticmachine merge upstream |
…browser-add-script-recorder-option
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
elastic#115184) * Add script recorder tab for browser based monitors * implement policy edit flow for script recorder * add tests and translations * adjust types * update metadata key and add test * merge master * adjust usePolicy and source_field * revert merge master * adjust types * use reusable hook * update zip url tls default fields * rename content constant * adjust error handling and remove ts-ignore * adjust error default value * remove unnecessary fragment * adjust types * update tech preview content Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
#115184) (#115698) * Add script recorder tab for browser based monitors * implement policy edit flow for script recorder * add tests and translations * adjust types * update metadata key and add test * merge master * adjust usePolicy and source_field * revert merge master * adjust types * use reusable hook * update zip url tls default fields * rename content constant * adjust error handling and remove ts-ignore * adjust error default value * remove unnecessary fragment * adjust types * update tech preview content Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dominique Clarke <[email protected]>
Summary
Fixes elastic/uptime#371
Adds the option to import scripts generated by the Elastic Synthetics Recorder.
Testing
Setting up your e2e testing environment
Overview: You will need to run a local version of kibana and package registry to test pending changes, but will use
elastic-package
to run a snapshot view of elasticsearch, fleet-server, and elastic-agent. Since fleet server relies on kibana, we'll also useelastic-package
to start a snapshot view of kibana exclusively for bootstrapping fleet-server, but will point our local version of kibana to a separate port. So we'll be running two versions of Kibana.kibana.dev.yml
settings, add removekibana.index
andxpack.task_manager.index
to remove legacy multitenancy features. This ensures that both our instances of Kibana can share saved objects related to fleet and agent. Also, add the following keys:env BUILD_TS_REFS_DISABLE=true yarn kbn bootstrap && yarn start
go get github.com/elastic/elastic-package
elastic-package stack up -d -v --services "elasticsearch" --version 8.0.0-SNAPSHOT
.kibana_system
usercurl -u elastic:changeme -X POST "http://localhost:9200/_security/user/kibana_system/_password?pretty" -H 'Content-Type: application/json' -d' { "password" : "changeme" } '
packages/synthetics
in the repo directory. Runelastic-package clean
, thenelastic-package build
.elastic-package stack up --version 8.0.0-SNAPSHOT -v --services "package-registry,kibana,elastic-agent,fleet-server"
in thepackages/synthetics
directory. This will start up elasticsearch, fleet-server, elastic-agent, package registry AND kibana. This is because fleet-server requires kibana for bootstrapping.Testing happy path
fleet/integrations/synthetics-0.3.1/add-integration
step
. There are a few you can use in the synthetics repo in theexamples/inline
directory if you have it pulled down, or you can use the script recorder to actually create a script.Testing invalid file
fleet/integrations/synthetics-0.3.1/add-integration
Testing non inline journey
fleet/integrations/synthetics-0.3.1/add-integration