backstage / backstage

Backstage is an open framework for building developer portals
https://backstage.io/
Apache License 2.0
28.23k stars 6k forks source link

fetch:cookiecutter: take into account template-local cookiecutter.json #4920

Closed vinayvinay closed 3 years ago

vinayvinay commented 3 years ago

Feature Suggestion

cookiecutter supports features that can be configured in cookiecutter.json, e.g. copy without render.

The suggestion is that fetch:cookiecutter takes a template-local cookiecutter.json into account.

values passed to fetch:cookiecutter will override those in the template-local cookiecutter.json, e.g. use_ci passed in from the scaffolder (UI) will override the static use_ci configured in the template-local cookiecutter.json.

Related discord chat

Possible Implementation

In templater/cookiecutter.ts, lookup templateContentsDir/cookiecutter.json and merge-in the auto-generated templateDir/cookiecutter.json.

Context

I'd like to make use of cookiecutter templates already in use within our organisation (before scaffolder) and keep their current behaviour as far as possible.

vinayvinay commented 3 years ago

This conversation moved slightly ahead in discord where @benjdlambert mentions:

right now anything that's in the template folder is the template

That's a bit of a surprise as cookiecutter repos are typically structured as https://github.com/spotify/cookiecutter-golang is. I'd love to be able to clone that into the workDir, and finally publish the generated {{cookiecutter.component_id}} directory to a fresh repo. If this has been discussed before, please point me to the conversation so I can catch-up (a quick search didn't reveal anything).

benjdlambert commented 3 years ago

So https://github.com/spotify/cookiecutter-golang is a legacy template still, not in the new v2 format. You'll see that it actually has a cookiecutter.json in it too which is used. https://github.com/spotify/cookiecutter-golang/blob/master/cookiecutter.json. But unfortunately, this wont work for v2 as we build that file automatically, and there's no more {{cookiecutter.component_Id}} as component_id doesn't make sense anymore as you can template anything. So using v1 templates, or the legacy version I think would work, but we need to solve v2.

There's two approaches I can think to solve this, we can place the cookiecutter.json under the ./template directory and just assume that if in the template directory there is a cookiecutter we should move that out of the skeleton and pass it to cookiecutter as config, or we can pass in the cookiecutter.json as options to publish action in v2 schema:

  - id: fetch
     name: fetch
     action: fetch:cookiecutter
     input: 
       config:
         _copy_without_render: ["some/glob"]

Or something similar to this?

vinayvinay commented 3 years ago

I see. So for v2 if we're staying with the decision that "./template contains only the template", then I'd vote for your suggestion to pass cookiecutter config as config to fetch:cookiecutter. That keeps it in sync with another design choice mentioned in (https://github.com/backstage/backstage/issues/4824#issuecomment-791045769, hooks as custom actions) where all config / hooks etc. will then be visible from the action config.

If there's more agreement, I'm happy to create a PR to keep moving this forward. In which case, let's head to the bike shed: do we want both config and values:

  - id: fetch
     name: fetch
     action: fetch:cookiecutter
     input: 
       config:
         _copy_without_render: ["some/glob"]
       values:
         name: '{{ parameters.name }}'

or, i'm guessing someone with little awareness of cookiecutter (e.g. myself) know that all these end up in a cookiecutter.json, shall we merge these and have:

  - id: fetch
     name: fetch
     action: fetch:cookiecutter
     input:
       config:  // (or explicitly call it cookiecutter.json) description: key-value pairs to be added to the generated cookiecutter.json
         name: '{{ parameters.name }}'
         _copy_without_render: ["some/glob"]
benjdlambert commented 3 years ago

Merging them all under config causes a backwards incompatible change, so i'd like not to do that if possible :) thoughts @freben @Rugvip ?

vinayvinay commented 3 years ago

ah yes, fair point about backwards incompatible change.

Rugvip commented 3 years ago

@vinayvinay @benjdlambert I was thinking that keeping config separate from the template content is bad because for example _copy_without_render is very coupled to the template contents. We're already setting values in the template definition though, so that point is a bit moot.

It seems like _copy_without_render is the only magic key that cookiecutter has atm, with the rest of the values just being used for the templating? I'd actually suggest the following:

  - id: fetch
     name: fetch
     action: fetch:cookiecutter
     input: 
       copyWithoutRender: ["some/glob"]
       values:
         name: '{{ parameters.name }}'

Especially with #4869 I'd argue that this makes it even more discoverable and easy to use than if we send people over to the cookiecutter config docs. It ofc means we need to stay in sync with cookiecutter, but I'd also imagine there's not a lot going on there atm. It also lets us be a bit more explicit about which config options we forward as cookiecutter's way of doing things doesn't always fit well with a templating service from a security point of view.

benjdlambert commented 3 years ago

@rugvip yep fine to me. That was the other way, but wasn't sure it was worth creating a mapping for it. Let's go with that then, copyWithoutRender input argument to the fetch:cookiecutter action 👍

vinayvinay commented 3 years ago

Sounds good @Rugvip. The only other cookiecutter "magic key" i could find is _extensions so might want to add that in too, while we're at it.

Slightly off topic but kinda related, if you have a moment, I'd love to learn more about the decision "./template only has the template" because cookiecutter supports multiple template directories under a single directory or repository. If we assume that the url always points to a single-template, we're forcing one-template-per-repo on the users of this action. Which is not the end of the world, if we support input.cliOptions or similar for this action, so that one can use --directory and use a cookiecutter monorepo.

freben commented 3 years ago

Isn't it quite unfortunate if you have an existing, actually functioning cookie cutter, but the scaffolder doesn't handle it by default without duplicating things into the template yaml?

Rugvip commented 3 years ago

@vinayvinay The templater had some quite magic logic to identify the output directory in the old format, it didn't support multiple template directories either. We also didn't want to allow for hooks to be used, as they're executed as scripts on the host machine which isn't ideal. Tbh this new way opens up for more options, because you can now have multiple directories next to the template yaml containing different templates if you need to, and then point to each of them in separate cookiecutter actions using the url input, ./template1, ./template2 etc.

@freben You can use existing ones, just that you need the slightly awkward {{cookiecutter.name}} folder name in the yaml as well as copy over the values from cookiecutter.json if there are any hardcoded ones. If needed we could add a way to import the values from cookiecutter.json, but tbh the template yaml is already not gonna support pointing to an existing cookiecutter template out of the box as you need to configure the parameters, so that'd probably just be bloat.

vinayvinay commented 3 years ago

I see, thanks for the context @Rugvip. I had the same feeling as @freben initially. I now see we want to make config explicit and have more control over the executable.

I'll follow-up with a PR to accept input.copyWithoutRender and input.extensions for the fetch:cookiecutter action.