#4205: update to prepare for emitting potentially stale events #4239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
Because the predata can be inferred to be the opposite, this gives as much information and renders more cleanly in the UI
This is probably better left for a later PR
thomasheartman
changed the title
sjaanus
reviewed
Jul 14, 2023
thomasheartman
added a commit
that referenced
this pull request
Jul 17, 2023
This PR activates the event emission that was prepared for in #4239. It emits events (behind a flag) when something is marked as potentially stale or the opposite. It takes the features returned from the store and creates events out of them. The events only contain data, no preData. This is because the preData can easily be inferred and because it gives a nicer event in the event log. Here is an image of the difference. The top event uses only data, so it shows the name of the feature and the new potentiallyStale status. The bottom event uses both preData and data, so it only shows the new potentiallyStale status and not the feature name (unless you show the raw event):  Should not be merged before #4239. Merge that and then rebase this off main or cherry the commit. ## Discussion ### `preData` Should we also use preData or is it enough to use only data? It seems unnecessary in this event, but I'm open to hearing your thoughts. ### event author: `createdBy` I've set `unleash-system` as the `createdBy` property on these events because they are generated by the system. I found the same string used some other places. However, it may be that there we want to use a different author.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds updates the potentially stale status change events whenever the potentially stale update function is run.
No events are emitted yet. While the emission is only a few lines of code, I'd like to do that in a separate PR so that we can give it the attention it deserves in the form of tests, etc.
This PR also moves the potentially stale update functionality from the
updatemethod to only being done in theupdatePotentiallyStaleFeaturesmethod. This keeps all functionality related to markingpotentiallyStalein one place.The emission implementation was removed in 4fb7cbd
The update queries
While it would be possible to do the state updates in a single query instead of three separate ones, wrangling this into knex proved to be troublesome (and would also probably be harder to understand and reason about). The current solution uses three smaller queries (one select, two updates), as Jaanus suggested in a private slack thread.
Input is welcome if you have any.