Skip to content
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

Edit Secure Variables as JSON #13461

Merged
merged 12 commits into from
Jul 4, 2022
Merged

Edit Secure Variables as JSON #13461

merged 12 commits into from
Jul 4, 2022

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Jun 22, 2022

  • Allows users to create and modify Secure Variables in JSON mode using Codemirror
  • Instead of using the existing (and now deprecated) IvyCodemirror, adds a {{code-mirror}} modifier with more robust error states (more in line with Vault)
  • Checks for JSON errors / lints on update, and only updates saveable values when there are no errors present.

TODO

  • Check for non-standard SV shape before saving. For example, a user can wrap their object in an array ([]) and it's still valid JSON, but we should warn them / prevent save.
  • Clean up code-mirror.js a bit; remove ruby mode, etc.
  • Find out why existing Ivy plugins are not inheriting color scheme
  • JSON on /new

image

@philrenaud philrenaud self-assigned this Jun 22, 2022
@philrenaud philrenaud linked an issue Jun 22, 2022 that may be closed by this pull request
1 task
@github-actions
Copy link

github-actions bot commented Jun 22, 2022

Ember Asset Size action

As of 8b3129c

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +5.2 kB +1.41 kB
nomad-ui.css +212 B +29 B
vendor.css +2.77 kB +1.09 kB

Files that got Smaller 🎉:

File raw gzip
vendor.js -191 kB -62.2 kB

@github-actions
Copy link

github-actions bot commented Jun 22, 2022

Ember Test Audit comparison

secure-variables 8b3129c change
passes 1326 1330 +4
failures 2 2 0
flaky 0 0 0
duration 000ms 000ms -000ms

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger
Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some potential bugs I see here that I've outlined. Also, because we're following atomic PRs, I haven't been able to give my input on API design, but I do make advocate for refactoring the save method.

I totally understand that adding tests could bloat this PR past 1,000 lines of code and you already know this work inside and out, but seeing tests would've helped me grok this more quickly and provide more meaningful feedback.

ui/app/components/secure-variable-form.js Outdated Show resolved Hide resolved
ui/app/components/secure-variable-form.js Outdated Show resolved Hide resolved
ui/app/components/secure-variable-form.js Show resolved Hide resolved
ui/app/components/secure-variable-form.js Show resolved Hide resolved
ui/app/components/secure-variable-form.js Outdated Show resolved Hide resolved
ui/app/components/secure-variable-form.hbs Show resolved Hide resolved
ui/app/modifiers/code-mirror.js Outdated Show resolved Hide resolved
ui/app/components/secure-variable-form.js Show resolved Hide resolved
* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup
@mikenomitch
Copy link
Contributor

Non-blocking issue when editing json: If I have two of the same key and then press delete on the comma at the end, the whole like will disappear.

This happened to me when I was copy/pasting to eventually change the dupe key, but I wanted to fix the json formatting before changing the key, but then the line disappeared.

a.mov

@mikenomitch
Copy link
Contributor

Also saving dupe keys with different values just made one disappear in non-JSON editing. Feels like this should be an error that blocks save rather than anything else.

@philrenaud
Copy link
Contributor Author

@mikenomitch duplicate keys and snap-json-to-top / delete block errors are both resolved.

The duplicate error currently shows up as a warning to the user, but doesn't prohibit save. I think this is a good first step.

@philrenaud philrenaud marked this pull request as ready for review July 4, 2022 20:31
@philrenaud philrenaud merged commit 50b75b6 into secure-variables Jul 4, 2022
@philrenaud philrenaud deleted the 13441-edit-as-json branch July 4, 2022 20:48
tgross pushed a commit that referenced this pull request Jul 8, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
tgross pushed a commit that referenced this pull request Jul 8, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
tgross pushed a commit that referenced this pull request Jul 11, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit as JSON
4 participants