This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
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.
Add a Cypress Test 🌲 #8295
Add a Cypress Test 🌲 #8295
Changes from 44 commits
8d22be3
0854ee0
6308954
f4f7cac
53a30ed
8312ead
e2be179
2c6265d
0ad99d7
358b692
7ab9222
67a59bd
2d8200b
b2adcf5
d9861a8
d08597d
322a750
02965aa
6c0acdb
20a6963
9f6128c
41030ec
b7c7f64
e2b7235
0ef494a
bf80b65
979b5ff
ea6f8af
2aca928
38bbb55
a1e9350
1e8953a
53b8a1c
1619330
b56da36
a78d212
4dd0545
ca86bad
cfdb2af
f82aa9a
d0cbae9
13ee082
58f32b7
d5d9aa7
dd0ff8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 assume we've weighed the risks 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.
we should already have a random string utility we can use?
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.
Mmm, true - I guess importing code from the app would be fine, but it feels like it's probably good hygiene to keep them completely separate, especially when the code in question is as simple as it is. Plus this is node-specific and we specifically want base64 encoding for the ed25519 key.
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.
20% sure this won't work on windows, but also a good chance that nodejs will have stubbed it
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.
Nodejs says, "on Windows only the write permission can be changed, and the distinction among the permissions of group, owner or others is not implemented." so sounds like it will work to some extent, or at least no-op.
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.
might be best to put this on a
try { } finally { ... }
, but not blocking for this PRThere 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.
👍