feat: add support for refreshKeys #12156

kissmikijr · 2022-06-20T19:59:41Z

Hey, I just made a Pull Request!

This is a potential implementation of the #12155 Nothing set in stone, would like some feedback on it!

✔️ Checklist

A changeset describing the change and affected packages. (more info)
Added or updated documentation
Tests for new functionality and regression tests for bug fixes
Screenshots attached (for UI changes)
All your commits have a Signed-off-by line in the message. (more info)

github-actions · 2022-06-20T20:00:03Z

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/plugin-catalog-backend	plugins/catalog-backend	minor	`v1.2.1-next.2`

github-actions · 2022-06-20T20:02:53Z

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

kissmikijr · 2022-06-20T20:05:35Z

plugins/catalog-backend/src/database/DefaultProcessingDatabase.ts

+    await Promise.all(
+      keys.map(k => {
+        return tx<DbRefreshKeysRow>('refresh_keys')
+          .insert({
+            entity_ref: stringifyEntityRef(k.entity),
+            key: k.key,
+          })
+          .onConflict(['entity_ref', 'key'])
+          .ignore();


I wasn't able to perform this with tx.batchInsert when I want to use an onConflict. Would it be better to check first if they exist and only do an insert if don't?

kissmikijr · 2022-06-20T20:06:27Z

plugins/catalog-backend/migrations/20220616202842_refresh_keys.js

+ */
+
+exports.up = async function up(knex) {
+  await knex.schema.createTable('refresh_keys', table => {


Not sure if this table should have a dedicated id field ?

jhaals

Thanks for this!

Not sure if I should comment on the RFC or the PRFC might be good to combine them next time 😅

jhaals · 2022-06-22T09:56:48Z

plugins/catalog-backend/src/database/DefaultProcessingDatabase.ts

+    const tx = txOpaque as Knex.Transaction;
+    const { keys } = options;
+
+    await Promise.all(


I believe we want to create a new set of refresh keys for the given entity in the database so that this method actually replacing the existing keys for that entity to not get into a situation where we potentially have an ever growing list of keys.

Given that this is probably better named setRefreshKeys

jhaals · 2022-06-22T09:58:37Z

plugins/catalog-backend/src/database/types.ts

@@ -81,6 +81,14 @@ export type ReplaceUnprocessedEntitiesOptions =
      type: 'delta';
    };

+export type RefreshKeyOptions = {
+  keys: { key: String; entity: Entity }[];


This should probably be entityRef instead of a full entity.

Could be, however in this case it would mean we'd need to emit the entityRefs from the processors, so the entityRef construction would happen inside all of the processors. With the current implementation they just emit an entity, and in the processing engine we can construct the refs based on the emitted entity.

I'm fine with both of them, maybe leaning towards the emission of entityRefs just for the sake of reducing the data thatis sent :D

kissmikijr · 2022-06-22T12:17:24Z

Not sure if I should comment on the RFC or the PRFC might be good to combine them next time 😅

Yeah I agree, I'll definietly combine them next time, it is a littlebit awkward this way. :/

kissmikijr · 2022-06-22T15:57:41Z

@jhaals With this my intention was to understand the problem space and try to explore the way it can be done, if you think in a general sense it looks okey, I'd move forward with adding tests, and making sure it passes the ci and move it to a regular PR.

Signed-off-by: Kiss Miklos <miklos@roadie.io>

plugins/catalog-backend/src/database/types.ts

Signed-off-by: Kiss Miklos <miklos@roadie.io>

Rugvip · 2022-07-06T08:11:31Z

plugins/catalog-backend/src/modules/core/PlaceholderProcessor.ts

+        processingResult.refresh(
+          `url:${relativeUrl({
+            key: resolverKey,
+            value: resolverValue,
+            baseUrl: location.target,
+            read,
+            resolveUrl,
+          })}`,
+        ),


I think this is gonna work for a lot of resolver implementations, but not all. I think it's worth an investigation into what it would mean to let the resolver return or emit refresh keys. Think for example an OpenAPI-aware resolver that follows $ref URLs that we then add to the set of refresh keys

Agree, that's gonna be more future-proof for sure! Let me see how would it look.

Rugvip · 2022-07-06T08:23:53Z

plugins/catalog-backend/src/modules/core/FileReaderProcessor.ts

            },
          })) {
            emit(parseResult);
+            emit(
+              processingResult.refresh(
+                `${LOCATION_TYPE}://${normalizedFilePath}`,


Sorry for flip-flopping, but we're leaning back towards using the serialized location format for refresh keys after all. Feels like that's the more future proof design that will let us add new behavior without awkwardness.

Think for example a refresh key that simply references a GitHub repo, we'd probably do something like github-repo:https://github.com/backstage/backstage for that since we need pretty much all parts of that URL anyway. If we just use the scheme it would need to either be the bare https://github.com/backstage/backstage, but there's a potential risk for overlap there. Another more explicit option is then github-repo://github.com/backstage/backstage, but that's where it starts getting real awkward, especially of we have values that don't really fit as URLs.

So with that, let's switch this back to file:<path> for now, as in drop the // 😅

Rugvip · 2022-07-06T08:25:01Z

plugins/catalog-backend/src/modules/core/UrlReaderProcessor.ts

@@ -93,6 +93,8 @@ export class UrlReaderProcessor implements CatalogProcessor {
          value: parseResults as CatalogProcessorEntityResult[],
        });
      }
+
+      emit(processingResult.refresh(`${location.type}:${location.target}`));


So right now leaning towards what's currently here, the same <type>:<target> format as we use for locations.

Signed-off-by: Kiss Miklos <miklos@roadie.io>

kissmikijr · 2022-07-06T15:09:57Z

@Rugvip Addressed the comments, opted to add the emit to the resolvers as the easiest solution right now. I can see some tests failed, but they pass locally, can it be that it is flaky?

Rugvip

Feeling we're getting pretty close to 😁

Rugvip · 2022-07-06T15:23:12Z

plugins/catalog-backend/api-report.md

@@ -567,6 +578,7 @@ export type PlaceholderResolverParams = {
  baseUrl: string;
  read: PlaceholderResolverRead;
  resolveUrl: PlaceholderResolverResolveUrl;
+  emit: CatalogProcessorEmit;


Yep, think this makes sense! 👍

@freben could you have a look as well to make sure you think this fits alright with how the placeholder processor is put together?

I'm thinking we should also call out in the changeset that custom placeholder resolvers should be updated to emit refresh keys.

Rugvip · 2022-07-06T15:30:45Z

plugins/catalog-backend/src/service/types.ts

  refresh(options: RefreshOptions): Promise<void>;
+  refreshByRefreshKeys(options: RefreshByRefreshKeysOptions): Promise<void>;


What do you think about making this an addition to the existing RefreshOptions rather than a new method?

If we won't expose the refreshKeys on the DefualtRefreshService we won't need to add the options to the existing RefreshOptions either, so I'll hold on to this and do it based on how we decide.

Rugvip · 2022-07-06T15:34:36Z

plugins/catalog-backend/src/service/DefaultRefreshService.ts

+  async refreshByRefreshKeys(options: RefreshByRefreshKeysOptions) {
+    await this.database.transaction(async tx => {
+      await this.database.refreshByRefreshKeys(tx, options);
+    });
+  }


I feel we need to do something different here tbh, just passing through without authorization is pretty unexpected. What I'm thinking is that we'd keep refreshing by keys internal, so it's not a thing you can request via the API. The only way to refresh via keys would be some form of direct runtime access for example via the entity providers API.

Have you considered this step yet? didn't see it in the RFC. How do you end up wanting to pipe refresh data into the catalog? Maybe we could even skip this part in the PR and leave it to a followup

Yea, I agree my initial idea was to just expose it on the refresh service and have its own endpoint. I can accept that we do not want to do this.

The other thing that seems kinda easy is to add the refreshing to the EntityProvider, they have access to the DefaultProcessingDatabase so should be easy to expose a new method that calls the appropriate refresh function. However, I don't think EntityProviders should handle refreshes, for me they should care about creating/deleting the entities. So I'd probably not put this behaviour on to the EntityProviders.

I think refreshing belongs to the catalog. So I am thinking about something like that the CatalogBuilder could return a refresh function and then everyone could decide how they want to use that to refresh their stuff.

stg like:

export default async function createPlugin( env: PluginEnvironment, ): Promise<Router> { const builder = await CatalogBuilder.create(env); builder.addProcessor(new ScaffolderEntitiesProcessor()); const { processingEngine, router, refresh } = await builder.build(); await processingEngine.start(); refresh(['refreshKey-1', 'refreshKey-2']) return router; }

I actually removed the possibility to trigger refreshes via the RefreshService to be able to tackle it in a separate PR if we decide it that way.

Signed-off-by: Kiss Miklos <miklos@roadie.io>

Rugvip

Nice, thanks! 👍

Shall we as is and leave the triggering of refreshes as a followup?

I think the builder addition could work, especially for use-cases like your own. Looking at #11611 this is very likely to be an extension point though, so I almost doesn't matter how we implement it for now as it'll be migrated to that anyway.

I would like us to explore the EntityProvider approach too though, making it possible for a provider to trigger refreshes through the connection. Or maybe even a new form of abstraction like some form of refresh sources

kissmikijr · 2022-07-07T11:48:39Z

Shall we as is and leave the triggering of refreshes as a followup?

I'd say so let's merge this one and I'll create a follow-up issue where we can discuss the different approaches for triggering the refreshes. I can explore both and create some PRs with the different approaches to discuss which fits better.

Rugvip · 2022-07-07T12:11:16Z

🎉

kissmikijr requested a review from a team as a code owner June 20, 2022 19:59

github-actions bot added awaiting-review area:catalog Related to the Catalog Project Area labels Jun 20, 2022

kissmikijr mentioned this pull request Jun 20, 2022

[RFC][catalog-backend] Refresh entities based on some pre stored key. #12155

Closed

kissmikijr commented Jun 20, 2022

View reviewed changes

jhaals reviewed Jun 22, 2022

View reviewed changes

backstage-goalie bot removed the awaiting-review label Jun 22, 2022

kissmikijr changed the title ~~[PRFC] - add support fot refreshKeys~~ [PRFC] - add support for refreshKeys Jun 22, 2022

github-actions bot added the awaiting-review label Jun 23, 2022

kissmikijr force-pushed the store-refresh-keys-to-entities branch from f34043e to 58ae6a5 Compare June 27, 2022 22:59

kissmikijr added 12 commits June 28, 2022 10:20

add a potential implementation for refreshKeys

def0eef

Signed-off-by: Kiss Miklos <miklos@roadie.io>

add refresh function to processingResult

2aa5bb6

Signed-off-by: Kiss Miklos <miklos@roadie.io>

Add refreshByRefreshKey to refresh service

6a0a4ec

Signed-off-by: Kiss Miklos <miklos@roadie.io>

addRefreshKEys -> setRefreshKeys

3744b0c

Signed-off-by: Kiss Miklos <miklos@roadie.io>

add deleteRefreshKey

2b1ab47

Signed-off-by: Kiss Miklos <miklos@roadie.io>

use entityRef instead of Entity

b713a4c

Signed-off-by: Kiss Miklos <miklos@roadie.io>

fix some tests

af0b031

Signed-off-by: Kiss Miklos <miklos@roadie.io>

fix existing tests

1e53a82

Signed-off-by: Kiss Miklos <miklos@roadie.io>

fix type from String to string

d653055

Signed-off-by: Kiss Miklos <miklos@roadie.io>

export CatalogProcessorRefreshKeysResult

bb58626

Signed-off-by: Kiss Miklos <miklos@roadie.io>

generate new api-report.md

3117584

Signed-off-by: Kiss Miklos <miklos@roadie.io>

fix UrlReaderProcessor test

3125f54

Signed-off-by: Kiss Miklos <miklos@roadie.io>

kissmikijr force-pushed the store-refresh-keys-to-entities branch from 6f915db to 3125f54 Compare June 28, 2022 08:23

kissmikijr requested a review from jhaals June 28, 2022 21:53

benjdlambert added this to In progress in Mammalian Mathematician Jun 29, 2022

Rugvip reviewed Jul 5, 2022

View reviewed changes

plugins/catalog-backend/src/database/types.ts Outdated Show resolved Hide resolved

kissmikijr added 5 commits July 5, 2022 14:40

use entity_id instead of entity_ref

e75bde1

Signed-off-by: Kiss Miklos <miklos@roadie.io>

use absolute urls

7c85fa3

Signed-off-by: Kiss Miklos <miklos@roadie.io>

fix tests

990d448

Signed-off-by: Kiss Miklos <miklos@roadie.io>

remove log based on condition

04f6715

Signed-off-by: Kiss Miklos <miklos@roadie.io>

add refreshKeys to the hashBuilder

d2bd286

Signed-off-by: Kiss Miklos <miklos@roadie.io>

kissmikijr requested a review from Rugvip July 5, 2022 22:33

Rugvip requested changes Jul 6, 2022

View reviewed changes

kissmikijr added 4 commits July 6, 2022 11:58

remove // from refreshKeys

a35aae4

Signed-off-by: Kiss Miklos <miklos@roadie.io>

pass emit to resolvers

3e3d828

Signed-off-by: Kiss Miklos <miklos@roadie.io>

add changeset

1dd6c22

Signed-off-by: Kiss Miklos <miklos@roadie.io>

add emit to resolvers

9a54bb6

Signed-off-by: Kiss Miklos <miklos@roadie.io>

kissmikijr requested a review from a team as a code owner July 6, 2022 14:34

fix url spelling

f3caf2e

Signed-off-by: Kiss Miklos <miklos@roadie.io>

kissmikijr requested a review from Rugvip July 6, 2022 15:10

Rugvip requested changes Jul 6, 2022

View reviewed changes

kissmikijr changed the title ~~[PRFC] - add support for refreshKeys~~ feat: add support for refreshKeys Jul 7, 2022

Rugvip added the needs discussion Bring up for discussion during next sync label Jul 7, 2022

kissmikijr added 2 commits July 7, 2022 12:03

remove refreshing by keys from refresh service

6714971

Signed-off-by: Kiss Miklos <miklos@roadie.io>

update changeset

1c23763

Signed-off-by: Kiss Miklos <miklos@roadie.io>

Mammalian Mathematician automation moved this from Review in progress to Reviewer approved Jul 7, 2022

Rugvip approved these changes Jul 7, 2022

View reviewed changes

Rugvip merged commit 9a03e9c into backstage:master Jul 7, 2022

Mammalian Mathematician automation moved this from Reviewer approved to Done Jul 7, 2022

kissmikijr mentioned this pull request Sep 7, 2022

feat: add refresh to EntityProviderConnection #13575

Merged

5 tasks

Rugvip mentioned this pull request Nov 11, 2022

📁 Catalog Improvement Meta Issue #14574

Open

31 tasks

kissmikijr mentioned this pull request Apr 12, 2023

Org Member: kissmikijr backstage/community#87

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for refreshKeys #12156

feat: add support for refreshKeys #12156

kissmikijr commented Jun 20, 2022 •

edited

github-actions bot commented Jun 20, 2022 •

edited

github-actions bot commented Jun 20, 2022

kissmikijr Jun 20, 2022

kissmikijr Jun 20, 2022

jhaals left a comment

jhaals Jun 22, 2022

jhaals Jun 22, 2022

kissmikijr Jun 22, 2022

kissmikijr commented Jun 22, 2022 •

edited

kissmikijr commented Jun 22, 2022

Rugvip Jul 6, 2022

kissmikijr Jul 6, 2022

Rugvip Jul 6, 2022

Rugvip Jul 6, 2022

kissmikijr commented Jul 6, 2022

Rugvip left a comment

Rugvip Jul 6, 2022

Rugvip Jul 6, 2022

kissmikijr Jul 7, 2022

Rugvip Jul 6, 2022

kissmikijr Jul 7, 2022

kissmikijr Jul 7, 2022

Rugvip left a comment

kissmikijr commented Jul 7, 2022

Rugvip commented Jul 7, 2022

		refresh(options: RefreshOptions): Promise<void>;
		refreshByRefreshKeys(options: RefreshByRefreshKeysOptions): Promise<void>;

feat: add support for refreshKeys #12156

feat: add support for refreshKeys #12156

Conversation

kissmikijr commented Jun 20, 2022 • edited

Hey, I just made a Pull Request!

✔️ Checklist

github-actions bot commented Jun 20, 2022 • edited

Changed Packages

github-actions bot commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhaals left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kissmikijr commented Jun 22, 2022 • edited

kissmikijr commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kissmikijr commented Jul 6, 2022

Rugvip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rugvip left a comment

Choose a reason for hiding this comment

kissmikijr commented Jul 7, 2022

Rugvip commented Jul 7, 2022

kissmikijr commented Jun 20, 2022 •

edited

github-actions bot commented Jun 20, 2022 •

edited

kissmikijr commented Jun 22, 2022 •

edited