[scaffolder-relation-processor] Template update pull requests

[djanickova]

Hey, I just made a Pull Request!

This PR introduces a new ability of scaffolder-relation-processor to automatically create PRs when a template is updated. Previously, when a template was updated to a new version, users could enable notifications in the config and would get notified about this update via the notification plugin in Backstage. With this feature, they can enable automatic pull requests and instead of just sending a notification - scaffolder-relation-processor will compare the template and the scaffolded repository and create a PR with necessary changes to update it to the new version.

This feature is disabled by default and can be enabled in config. It is separate from the notification feature - users can enable one of them, both or neither.

To better understand how this works, please take a look at the updated README which describes the process.

This branch consists of 3 separate merged PRs which were already reviewed, you can take a look at them here:
djanickova#7
djanickova#13
djanickova#14

An example of a PR that was created using this feature can be found here: djanicko-test-org/reactrepo#32.

Implements https://issues.redhat.com/browse/RHDHPLAN-479.

✔️ 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)

[backstage-goalie]

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.

[backstage-goalie]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage-community/plugin-catalog-backend-module-scaffolder-relation-processor	workspaces/scaffolder-relation-processor/plugins/catalog-backend-module-scaffolder-relation-processor	minor	v2.10.0

[dzemanov]

Thank you @djanickova, PR looks great!

I retested again these scenarios:

1. both pull requests and notifications disabled works correctly
2. only notifications enabled works correctly
3. only pull requests enabled works correctly
4. multiple entities getting PR update in one relation processor run
5. both pull requests and notifications enabled works correctly

• template update on GitLab correctly creates PR for repo on GitHub and GitLab
• template update on GitHub correctly creates PR fro repo on GitHub and GitLab

6. Tested default messages + titles and custom messages + titles work correctly and ENTITY_DISPLAY_NAME and PR_LINK are substituted correctly
7. Tested handling errors
   • For GitLab, when the branch is already created, I think we only show Bad request as error (as opposed to GitHub informing about branch already existing) as more info is under cause, I think it's alright since the situation should not happen normally. We could instead of error provide some param for PR creation to update in case branch is existing if possible.
8. Tested when template version updates but no changes appeared works correctly.
9. Tested variable replacement logic

• If i understand correctly, we are replacing only values in lines that have key:value structure, template contains some variable in value and keys match between template file and scaffolded file. This is good enough, we can improve later if needed. Maybe we could update TEMPLATE_VARIABLE_REGEX to also detect something like this: ${{ parameters.repoUrl | parseRepoUrl }}.
• I think errors could be introduced with the logic of creating global key:value scaffoldedMap for all scaffolded files - if another file has the same key with some value with variable, even if it is a different variable, it gets replaced with the first occurrence that was in totally different place. Same problem might appear when keys with the same names appear within the same file, like:

metadata:
  description: ${{ values.metadataDescription }}
spec:
  description: ${{ values.specDescription }}

I don't think the same file keys are likely, but maybe we can improve by having the map contain top level of filename -> key:value if needed, although these cases might not be likely to surface as well.

GitHub

github_compressed.mov

GitLab

gitlab_compressed.mov

Multiple at once

Error (when default message and when custom message)

• Link here doesn't look very nice but unfortunately I don't think there is an option to render it nicer in notifications

[djanickova]

Thank you for your thorough review @dzemanov! Regarding the second bullet point in number 9, the preprocessTemplate function is called for each file being compared, so having the same key:value pairs across different files should not be an issue. However, the limitation regarding the same keys in a single file is valid - one option of how to mitigate it could be to include the parent of the key, so eg. for

metadata:
  description: ${{ values.metadataDescription }}
spec:
  description: ${{ values.specDescription }}

one keys in the map would be metadata.description and spec.description. This would introduce more complexity as we would have to account for the indentation, but we can discuss whether this would be worth implementing.

Regarding the first bullet point of 9, adding ${{ parameters.repoUrl | parseRepoUrl }} is very problematic as this would heavily depend on whether the actual content in the scaffolded repo is on the same line or not. I believe for situations where it is not (and we cannot know this just be regexing), this would just create more incorrect additions.

I think in the future, the best way of improving this diffing would be to enhance the task api so that it could take a template entity as a filter. That way, we could fetch the inputs that were used to scaffold the entity and use them instead of the complicated diffing logic that we are using now. The question remains, however, if this would be better for users - they may have changed some of the values since scaffolding, so we would replace them with the outdated ones.

Personally, I would say that we should go with the logic we have right now and see what feedback we receive. If people prefer having the inputs given during the scaffolding process, we could further work on that.

Regarding the link in the notification - you are correct, unfortunately there is no way of formatting it better, it would be great if it was clickable.

[dzemanov]

Regarding the second bullet point in number 9, the preprocessTemplate function is called for each file being compared,

You are right, I totally missed that, thx!

the limitation regarding the same keys in a single file is valid...we should go with the logic we have right now and see what feedback we receive

Yeah, I agree we should not complicate diffing and go with current implementation, as the cases will not occur often and users can manually adjust if needed. If we receive feedback about improving variable replacement, the best approach is to look at task api for sure, as you have mentioned.

adding ${{ parameters.repoUrl | parseRepoUrl }} is very problematic as this would heavily depend on whether the actual content in the scaffolded repo is on the same line or not

👍🏻