aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.6k stars 3.9k forks source link

aws-cdk-lib(core): ILocalBundling is missing access to asset source path #23439

Open fbenkstein opened 1 year ago

fbenkstein commented 1 year ago

Describe the bug

When a ILocalBundling's tryBundle is called there is no way to find out the asset's source path from within the function. The tryBundle function only receives the output dir and the bundling options:

export interface ILocalBundling {
  /**
   * This method is called before attempting docker bundling to allow the
   * bundler to be executed locally. If the local bundler exists, and bundling
   * was performed locally, return `true`. Otherwise, return `false`.
   *
   * @param outputDir the directory where the bundled asset should be output
   * @param options bundling options for this asset
   */
  tryBundle(outputDir: string, options: BundlingOptions): boolean;
}

I was able to work around this problem by passing the asset path to my classes constructur but I don't think that is the intended way to call it (pardon the Python - I am not very versed in TypeScript):

@jsii.implements(ILocalBundling)
class MyLocalBundling:
  def __init__(self, *, asset_path):
      self.asset_path = asset_path

  def try_bundle(self, output_dir, *, bundling_options):
    # TODO: put self.asset_path into output_dir somehow
    pass

class MyStack(Stack):
  def add_my_asset(self):
    my_asset_path = my_get_asset_path()
    asset = Asset(
        self,
        "MyAsset",
        path=my_asset_path,
        bundling=BundlingOptions(
           local=MyLocalBundling(asset_path=asset_path),
           # other options, like docker image etc. go here
           ...
        )
    )

This is needlessly complicated (because I have to create a local variable for the asset path) and potentially error-prone (I might pass the wrong asset path to the bundler). If this is indeed the intended behaviour I wish this was called out in the documentation.

I suggest extending the ILocalBundling interface with an additional, optional method:

  /**
   * This method is called before attempting docker bundling to allow the
   * bundler to be executed locally. If the local bundler exists, and bundling
   * was performed locally, return `true`. Otherwise, return `false`.
   *
   * @param assetPath the path where the input asset is found
   * @param outputDir the directory where the bundled asset should be output
   * @param options bundling options for this asset
   */
  tryBundleAsset?(assetPath: string, outputDir: string, options: BundlingOptions): boolean;

The asset staging logic could try to call tryBundleAsset if it exists and if not fall back to tryBundle.

Expected Behavior

The asset path should be a parameter to the tryBundle method of a ILocalBundling

Current Behavior

An ILocalBundling has no way of knowing the asset path unless it's passed in the constructure or through a closure.

Reproduction Steps

See above.

Possible Solution

See above.

Additional Information/Context

No response

CDK CLI Version

2.55.0 (build 077d77d)

Framework Version

No response

Node.js Version

v14.21.1

OS

Linux

Language

Typescript, Python

Language Version

Python 3.8

Other information

No response

peterwoodworth commented 1 year ago

If you have an idea on how to implement this, a PR would be a great jump start for this

gscpw commented 1 year ago

This is still a problem; it is a significant usability oversight.