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

Fix error updating secrets #4428

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Fix error updating secrets #4428

merged 1 commit into from
Aug 28, 2024

Conversation

jkeiser
Copy link
Contributor

@jkeiser jkeiser commented Aug 26, 2024

Right now, if you update a secret in-place and then quickly click on a component that uses it (after rebase but before DVU), an error toast pops up saying it couldn't find the secret.

This is because when we get the property values for a component for editing, we try to look up the secret by matching the key stored in the dependent value (cached in the node) against the keys in all Secret node. Since the secret key has since been updated, the secret can't be found. It eventually gets better, but it happens enough to drive John Watson crazy.

This change gets us past the error by sending back null (telling the frontend it's not set). Going forward, we'll talk through how to handle situations where dependent values aren't up to date with @wendybujalski and @jobelenus -- there is a general problem to solve there, as @jhelwig points out. And we should consider whether we should include more information (such as the secret id) in the JSON value for secrets, to reduce how often this will affect people. (Even if we did that, the problem would persist when the secret is deleted.)

This only affects the property editor value calculation.

Fixes BUG-481.

@github-actions github-actions bot added the A-dal label Aug 26, 2024
@jkeiser jkeiser force-pushed the jkeiser/secret-update-error branch 3 times, most recently from 1ca44d1 to a859ea3 Compare August 27, 2024 23:54
@jobelenus
Copy link
Contributor

@jkeiser Thanks for the call out here! The only remaining thing on my mind here is, perhaps instead of debug! when we hit this null case—do we want to add an error! so that we can measure in honeycomb how often this exact problem is effecting people? That might help us prioritize when/how much energy to put into a better resolution here?

@jkeiser jkeiser force-pushed the jkeiser/secret-update-error branch 2 times, most recently from c901388 to 45e03e6 Compare August 28, 2024 17:55
Previously we got the secret_id by reading the
JavaScript dependent value key, and figuring out
which secret had that key. This causes issues when
DVUs are out of date. Now we follow identity
functions all the way back to the secret and get
the secret_id that way.
@jkeiser jkeiser force-pushed the jkeiser/secret-update-error branch from 45e03e6 to a2e48b7 Compare August 28, 2024 18:42
@jkeiser jkeiser added this pull request to the merge queue Aug 28, 2024
Merged via the queue into main with commit f91836a Aug 28, 2024
8 checks passed
@jkeiser jkeiser deleted the jkeiser/secret-update-error branch August 28, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants