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

Scaffolder: Renders path templates when copyWithoutRender is used #12491

Merged

Conversation

namco1992
Copy link
Contributor

When putting templated paths in copyWithoutRender filter, like
{{project}}/something, the path template won't get rendered and the
files are copied to the exact {{project}}/something instead of
someproject/something.

Cookiecutter has fixed this issue in cookiecutter/cookiecutter#456. For users who come from the Cookiecutter world, this inconsistency between Cookiecutter and Backstage is unexpected and could lead to some confusion.

I believe the behaviour in Backstage should be consistent with Cookiecutter. Note that this change affects templating even not in cookiecutterCompat mode, I'm not sure if this would be considered as a breaking change. However, I think having different behaviours (render paths in cookiecutterCompat mode, otherwise ignore it) might be even more confusing, and I can't really think of a use case that you actually want to have untemplated paths in your end result.

Signed-off-by: Mengnan Gong namco1992@gmail.com

Hey, I just made a Pull Request!

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

@namco1992 namco1992 requested review from a team as code owners July 7, 2022 01:13
@github-actions github-actions bot added the area:scaffolder Everything and all things related to the scaffolder project area label Jul 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2022

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder-backend plugins/scaffolder-backend patch v1.4.0-next.3

@jhaals jhaals added the needs discussion Bring up for discussion during next sync label Jul 7, 2022
jhaals
jhaals previously requested changes Jul 7, 2022
Copy link
Member

@jhaals jhaals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

I agree it's a bit strange that file names aren't templated. I think we want this to be the default in the long run but we do consider this to be a breaking change. There's probably some user using backstage to template templates.

What I think we should do is to make sure that the cookiecutterCompat template file names and we consider that a bugfix.

copyWithoutRender should probably be deprecated and replaced by a new mode which does template the names but not the content. Do you have any name suggestions for this?
copyWithoutTemplating or copyNoRender is just some names that popped up in my head.

@namco1992
Copy link
Contributor Author

namco1992 commented Jul 8, 2022

Hi @jhaals, thanks for the review!

First of all, I agree with you that the baseline should be the cookiecutterCompat template file names.

copyWithoutRender should probably be deprecated and replaced by a new mode which does template the names but not the content. Do you have any name suggestions for this?

I think the name also comes from the Cookiecutter context. To make sure I understand it correctly, what you are suggesting is:

  1. copyWithoutRender behaviour won't change, regardless of the cookiecutterCompat
  2. have a new field (name TBD) that should render paths and ignore content templates

If above is the correct understanding, I think there might also be some confusion:

  1. For users who come from Cookiecutter, the copyWithoutRender might not work as they expected. They have the exact same flag _copy_without_render in Cookiecutter, but it represent different behaviours in Backstage.
  2. I also don't have a better idea about the naming of the new field. IMO neither copyWithoutTemplating nor copyNoRender is clear enough for the user to understand what it does and how it's different from copyWithoutRender. They proposed to introduce _copy_without_render_paths in Added: Path templates will be rendered when copy_without_render used cookiecutter/cookiecutter#839, but they didn't go for it.

In terms of the backward compatibility, maybe we can view it by listing out who might be affected, ✅ means all good, ❌ means breaking change.

  1. The user from Cookiecutter ✅
  2. The user who uses Backstage templating
    a. expects Cookiecutter compatible behaviour ✅
    b. expects the current behaviour ❌

I would argue that the users that get affected might be minimal, and I can't think of a use case that you actually want to have untemplated paths in your end result even if you are using the Backstage templating. Cookiecutter's fix is also considered a "breaking change" since they just changed the way how _copy_without_render works, but it seems to be a rather smooth transition. However, I don't have any data to back my guess and I might be very wrong about it.

@Rugvip Rugvip assigned benjdlambert and unassigned jhaals Jul 11, 2022
@benjdlambert
Copy link
Member

Hey! 👋

@jhaals is out on vacation, so I'm gonna try and help get this through 👍

would argue that the users that get affected might be minimal, and I can't think of a use case that you actually want to have untemplated paths in your end result even if you are using the Backstage templating.

So imagine the use case that you want a template that can create you other software templates for development. You don't want the paths templating as they're only meant to be templated when the created template is run. It's a little bit of a meta usage, but I know that theres adopters using this, so unfortunately it would break that functionality so we need to be respective of that.

I'm not to concerned of keeping with the cookiecutter conventions per se, it's also quite possible that in the future we will drop the support for cookiecutter out of the fetch:template action, and point people to using the cookiecutter module instead, as this shells out to an installed version of cookiecutter. It's going to make this a little bit easier to reason about this package.

I think the way forward should be as so:

  1. Deprecate copyWithoutRender regardless of mode, adding a log message when it is passed to get people to use the new one.
  2. Introduce a copyWithoutTemplating option with the new behaviour. (Or if you can find a better name, go ahead 🙏)
  3. Create a ticket to deprecate the cookiecutterCompat mode, so that we can move people over to using the module instead.

A little bit of background about the cookiecutterCompat mode is that we wanted to try and make the move from fetch:cookiecutter to fetch:template pretty smooth, so added this option so that it could be pretty transparent and not require too much effort.

Does this make sense?

@namco1992
Copy link
Contributor Author

Hi @benjdlambert, thanks for the help! I have a bit more context now thanks to you 😄 . Yes, "templating another template" could be a valid use case.

Initially I thought the cookiecutterCompat mode would be the way to go for the users who migrated from Cookiecutter, so I was concerned about the consistency of the naming and all. I had the impression that Cookiecutter has a larger user base in terms of templating, so I was trying to tweak the Backstage to fit in Cookiecutter's shoes. I think we can let go of it if we just go with the cookiecutter module in the future, but I would miss the convenience of having the Cookiecutter support but no extra dependencies to the backend 😂.

I will go with your suggestions and create a ticket to deprecate the cookiecutterCompat mode once this issue is closed. Thanks!

@namco1992 namco1992 force-pushed the render-path-copy-without-render branch from 3c3c307 to 6fdbccb Compare July 13, 2022 07:58
@github-actions github-actions bot added auth area:catalog Related to the Catalog Project Area area:techdocs Related to the TechDocs Project Area documentation Improvements or additions to documentation labels Jul 13, 2022
@namco1992 namco1992 force-pushed the render-path-copy-without-render branch from 6fdbccb to 43c972d Compare July 13, 2022 08:05
@github-actions github-actions bot removed documentation Improvements or additions to documentation area:catalog Related to the Catalog Project Area auth area:techdocs Related to the TechDocs Project Area labels Jul 13, 2022
@namco1992
Copy link
Contributor Author

Hi @benjdlambert,

I've updated the PR to reflect what you suggested, pls help review, thanks!

PS. sorry for the forks who got mis-tagged in the PR, I messed up my branch and the bot added you, but I don't think I can revert it 😂

Add `copyWithoutTemplating` to the the fetch template action input.
`copyWithoutTemplating` also accepts an array of glob patterns. Contents
of matched files or directories are copied without being processed, but
paths are subject to rendering.

Deprecate `copyWithoutRender` in favor of `copyWithoutTemplating`.

Signed-off-by: Mengnan Gong <namco1992@gmail.com>
@namco1992 namco1992 force-pushed the render-path-copy-without-render branch from 43c972d to ff316b8 Compare July 13, 2022 13:05
@benjdlambert benjdlambert removed the needs discussion Bring up for discussion during next sync label Jul 14, 2022
Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! and the patience 🙏

@benjdlambert benjdlambert merged commit 4df3d24 into backstage:master Jul 14, 2022
@namco1992 namco1992 deleted the render-path-copy-without-render branch July 15, 2022 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scaffolder Everything and all things related to the scaffolder project area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants