This PR updates the returned value about segments to also include the CR
title and to be one list item per strategy per change request. This
means that if the same strategy is used multiple times in multiple
change requests, they each get their own line (as has been discussed
with Nicolae).
Because of this, this pr removes a collection step in the query and
fixes some test cases.
The previous check would return `false` if the value was 0, causing a
bug where the usage data wouldn't be included.
This also adds tests to ensure that usage data for CR segments is
propagated correctly because that's where I first encountered the issue.
Before this fix, if the values were 0, the data would display like the
bottom element in the screenshot:
This PR changes the behavior of the API a little bit. Instead of
removing any strategies from `changeRequestStrategies` that are also
in `strategies`, we keep them in instead.
The reason for this is that the overview of where a segment is used is
incomplete if it shows only strategies but not CRs. Imagine this:
You want to delete a segment, but you're told it's only used in strategy
S.
So you go and remove it from strategy S, but then you're told it's
suddenly used in CRs A, B, and C. This is now a two-step operation
with a bad surprise. Instead, we could show you immediately that this
segment is used in strategy S and CRs A, B, and C.
This PR handles the case where a single strategy is used in multiple
change requests. Instead of listing the strategy several times in the
output, we consolidate the entries and add a new `changeRequestIds`
property. This is a non-empty list that points to all the change
requests it is used in.
This is required for us to be able to link back to the change requests
from the UI overview.
This PR changes the payload of the strategiesBySegment endpoint when the
flag is active. In addition to returning just the strategies, the object
will also contain a new property, called `changeRequestStrategies`
containing the strategies that are used in change requests.
This PR does not update the schema. That can be done later when the
changes go into beta. This also allows us some time to iterate on the
payload without changing the public API.
## Discussion points:
Should `strategies` and `changeRequestStrategies` ever contain
duplicates? Take this scenario:
- Strategy S uses segment T.
- There is an open change request that updates the list of segments for
S to T and a new segment U.
- In this case, strategy S would show up both in `strategies` _and_ in
`changeRequestStrategies`.
We have two options:
1. Filter the list of change request strategies, so that they don't
contain any duplicates (this is currently how it's implemented)
2. Ignore the duplicates and just send both lists as is.
We're doing option 2 for now.
Removing a user from a project was impossible if you only had 1 owner.
It worked fine when having more than an owner. This should fix it and
we'll add tests later
This PR adds the ability to detect which strategies use a specific
segment in active change requests.
It does not wire this functionality up to anything just yet. Follow-up
PRs will integrate this with the segment service and eventually with the
front end.
This PR updates the segment usage counting to also include segment usage
in pending change requests.
The changes include:
- Updating the schema to explicitly call out that change request usage
is included.
- Adding two tests to verify the new features
- Writing an alternate query to count this data
Specifically, it'll update the part of the UI that tells you how many
places a segment is used:
## Implementation
Implementing this was a little tricky. Previously, we'd just count
distinct instances of feature names and project names on the
feature_strategy table. However, to merge this with change request data,
we can't just count existing usage and change request usage separately,
because that could cause duplicates.
Instead of turning this into a complex DB query, I've broken it up into
a few separate queries and done the merging in JS. I think that's more
readable and it was easier to reason about.
Here's the breakdown:
1. Get the list of pending change requests. We need their IDs and their
project.
2. Get the list of updateStrategy and addStrategy events that have
segment data.
3. Take the result from step 2 and turn it into a dictionary of segment
id to usage data.
4. Query the feature_strategy_segment and feature_strategies table, to
get existing segment usage data
5. Fold that data into the change request data.
6. Perform the preexisting segment query (without counting logic) to get
other segment data
7. Enrich the results of the query from step 2 with usage data.
## Discussion points
I feel like this could be done in a nicer way, so any ideas on how to
achieve that (whether that's as a db query or just breaking up the code
differently) is very welcome.
Second, using multiple queries obviously yields more overhead than just
a single one. However, I do not think this is in the hot path, so I
don't consider performance to be critical here, but I'm open to hearing
opposing thoughts on this of course.
This PR hooks up the changes introduced in #5301 to the API and puts
them behind a feature flag. A new test has been added and the test setup
has been slightly tweaked to allow this test.
When the flag is enabled, the API will now not let you delete a segment
that's used in any active CRs.
Switch the express-openapi implementation from our internal fork to the
upstream version. We have upstreamed our changes and a new version has
been released, so this should be the last step before we can retire our
fork.
Because some of the dependencies have been updated since our internal
fork, we also need to update some of our error handling to reflect this.
This PR adds a cleanup job that removes unknown feature flags from
last_seen_at_metrics table every 24 hours since we no longer have a
foreign key on the name column in the features table.
## About the changes
This makes sure that projects have at least one owner, either a group or
a user. This is to prevent accidentally losing access to a project.
We check this when removing a user/group or when changing the role of a
user/group
**Note**: We can still leave a group empty as the only owner of the
project, but that's okay because we can still add more users to the
group
This PR is the first step in separating the client and admin stores.
Currently our feature toggle services uses the client store to serve
multiple purposes.
Admin API uses the feature toggle service to serve both the feature
toggle list and playground features, while the client API uses the
feature toggle service to serve client features. The admin API can
change often and have very different requirements than the client API,
which changes infrequently and generally keeps the same stable structure
for long periods of time. This architecture is error prone, because when
you need to make changes to the admin API, you can very easily affect
the client API.
I aim to put up a stone wall between the two APIs. Complete separation
between the two APIs, at the cost of some duplication.
In this PR I have created a feature oriented architecture for client
features and disconnected the client API from the feature toggle
service. It now goes through it's own service to it's own store. For
feature toggle service I have duplicated and replaced the functionality
that serves /api/admin/features, I have kept a lot of the ugliness in
the code and haven't removed anything in order to avoid breaking
changes.
Next steps:
* Move playground to admin API
* Remove client-feature-toggle-store from feature-toggle-service
Fixes an issue where SSO group sync would delete a syncable group that a
user was manually added to
## Discussion points
Is this the longterm fix for this? Or would we want another column in
the mapping table for future-proofing this?
https://linear.app/unleash/issue/2-1253/add-support-for-more-events-in-the-slack-app-integration
Adds support for a lot more events in our integrations. Here is how the
full list looks like:
- ADDON_CONFIG_CREATED
- ADDON_CONFIG_DELETED
- ADDON_CONFIG_UPDATED
- API_TOKEN_CREATED
- API_TOKEN_DELETED
- CHANGE_ADDED
- CHANGE_DISCARDED
- CHANGE_EDITED
- CHANGE_REQUEST_APPLIED
- CHANGE_REQUEST_APPROVAL_ADDED
- CHANGE_REQUEST_APPROVED
- CHANGE_REQUEST_CANCELLED
- CHANGE_REQUEST_CREATED
- CHANGE_REQUEST_DISCARDED
- CHANGE_REQUEST_REJECTED
- CHANGE_REQUEST_SENT_TO_REVIEW
- CONTEXT_FIELD_CREATED
- CONTEXT_FIELD_DELETED
- CONTEXT_FIELD_UPDATED
- FEATURE_ARCHIVED
- FEATURE_CREATED
- FEATURE_DELETED
- FEATURE_ENVIRONMENT_DISABLED
- FEATURE_ENVIRONMENT_ENABLED
- FEATURE_ENVIRONMENT_VARIANTS_UPDATED
- FEATURE_METADATA_UPDATED
- FEATURE_POTENTIALLY_STALE_ON
- FEATURE_PROJECT_CHANGE
- FEATURE_REVIVED
- FEATURE_STALE_OFF
- FEATURE_STALE_ON
- FEATURE_STRATEGY_ADD
- FEATURE_STRATEGY_REMOVE
- FEATURE_STRATEGY_UPDATE
- FEATURE_TAGGED
- FEATURE_UNTAGGED
- GROUP_CREATED
- GROUP_DELETED
- GROUP_UPDATED
- PROJECT_CREATED
- PROJECT_DELETED
- SEGMENT_CREATED
- SEGMENT_DELETED
- SEGMENT_UPDATED
- SERVICE_ACCOUNT_CREATED
- SERVICE_ACCOUNT_DELETED
- SERVICE_ACCOUNT_UPDATED
- USER_CREATED
- USER_DELETED
- USER_UPDATED
I added the events that I thought were relevant based on my own
discretion. Know of any event we should add? Let me know and I'll add it
🙂
For now I only added these events to the new Slack App integration, but
we can add them to the other integrations as well since they are now
supported.
The event formatter was refactored and changed quite a bit in order to
make it easier to maintain and add new events in the future. As a
result, events are now posted with different text. Do we consider this a
breaking change? If so, I can keep the old event formatter around,
create a new one and only use it for the new Slack App integration.
I noticed we don't have good 404 behaviors in the UI for things that are
deleted in the meantime, that's why I avoided some links to specific
resources (like feature strategies, integration configurations, etc),
but we could add them later if we improve this.
This PR also tries to add some consistency to the the way we log events.
This commit changes our linter/formatter to biome (https://biomejs.dev/)
Causing our prehook to run almost instantly, and our "yarn lint" task to
run in sub 100ms.
Some trade-offs:
* Biome isn't quite as well established as ESLint
* Are we ready to install a different vscode plugin (the biome plugin)
instead of the prettier plugin
The configuration set for biome also has a set of recommended rules,
this is turned on by default, in order to get to something that was
mergeable I have turned off a couple the rules we seemed to violate the
most, that we also explicitly told eslint to ignore.
## About the changes
This fixes a bunch of openHandles from our tests
I've used this script to find out the ones that leave them:
`find src -name "*.test.ts" -printf "%f\n" | xargs -i sh -c "echo =====
{} && yarn test {}"`
If there's an issue, the script will halt and the last filename will be
the one that has to be fixed.
Each commit fixes one problem so it's easy to review
https://linear.app/unleash/issue/2-1403/consider-refactoring-the-way-tags-are-fetched-for-the-events
This adds 2 methods to `EventService`:
- `storeEvent`;
- `storeEvents`;
This allows us to run event-specific logic inside these methods. In the
case of this PR, this means fetching the feature tags in case the event
contains a `featureName` and there are no tags specified in the event.
This prevents us from having to remember to fetch the tags in order to
store feature-related events except for very specific cases, like the
deletion of a feature - You can't fetch tags for a feature that no
longer exists, so in that case we need to pre-fetch the tags before
deleting the feature.
This also allows us to do any event-specific post-processing to the
event before reaching the DB layer.
In general I think it's also nicer that we reference the event service
instead of the event store directly.
There's a lot of changes and a lot of files touched, but most of it is
boilerplate to inject the `eventService` where needed instead of using
the `eventStore` directly.
Hopefully this will be a better approach than
https://github.com/Unleash/unleash/pull/4729
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
We love all open-source Unleash users. in 2022 we built the [segment
capability](https://docs.getunleash.io/reference/segments) (v4.13) as an
enterprise feature, simplify life for our customers.
Now it is time to contribute it to the world 🌏
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.io>
## About the changes
This enables us to use names instead of permission ids across all our
APIs at the computational cost of searching for the ids in the DB but
improving the API user experience
## Open topics
We're using methods that are test-only and circumvent our business
logic. This makes our test to rely on assumptions that are not always
true because these assumptions are not validated frequently.
i.e. We are expecting that after removing a permission it's no longer
there, but to test this, the permission has to be there before:
78273e4ff3/src/test/e2e/services/access-service.e2e.test.ts (L367-L375)
But it seems that's not the case.
We'll look into improving this later.
## About the changes
- `getActiveUsers` is using multiple stores, so it is refactored into
read-model
- Refactored Instance stats service into `features` to co-locate related
code
Closes https://linear.app/unleash/issue/UNL-230/active-users-prometheus
### Important files
`src/lib/features/instance-stats/getActiveUsers.ts`
## Discussion points
`getActiveUsers` is coded less _class-based_ then previous similar
read-models. In one file instead of 3 (read-model interface, fake read
model, sql read model). I find types and functions way more readable,
but I'm ready to refactor it to interfaces and classes if consistency is
more important.
This test enforces that descriptions and examples are cleared if they
are not present in the feature naming data.
This behavior is already present, but it hasn't been encoded in any
tests before.
This PR updates the back-end handling of feature naming patterns to add
implicit leading `^`s and trailing `$`s to the regexes when comparing
them.
It also adds tests for the new behavior, both for new flag names and for
examples.
## Discussion points
Regarding stripping incoming ^ and $: We don't actually need to strip
incoming `^`s and `$`s: it appears that `^^^^^x$$$$$` is just as valid
as `^x$`. As such, we can leave that in. However, if we think it's
better to strip, we can do that too.
Second, I'm considering moving the flag naming validation into a
dedicated module to encapsulate everything a little better. Not sure if
this is the time or where it would live, but open to hearing
suggestions.
This PR adds feature name pattern validation to the import validation
step. When errors occur, they are rendered with all the offending
features, the pattern to match, plus the pattern's description and
example if available.
To achieve this I've added an extra method to the feature toggle service
that checks feature names without throwing errors (because catching `n`
async errors in a loop became tricky and hard to grasp). This method is
also reused in the existing feature name validation method and handles
the feature enabled chcek.
In doing so, I've also added tests to check that the pattern is applied.
Does what it says on the tin, should help with cleaning up
https://github.com/Unleash/unleash/pull/4512 and respective schema
changes.
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
Adds a first iteration of feature flag naming patterns. Currently behind a flag.
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.io>
Co-authored-by: andreas-unleash <andreas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
