feat: Response Header support

sandeep-paliwal commented 1 year ago

DO NOT MERGE Support to specify response headers, by adding them to S3 metadata. Rules to apply response headers are read from config and are applied based on order they were specified.

Note: This change is dependent on - https://github.com/adobe/aio-cli-lib-app-config . Util changes in app config lib are released and referenced inside web lib these changes should not be merged.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Manual e2e and automated unit tests

Screenshots (if appropriate):

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] I have signed the Adobe Open Source CLA.
[x] My code follows the code style of this project.
[] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

codecov[bot] commented 1 year ago

Codecov Report

Merging #179 (d8e9684) into master (c8c0d1d) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #179   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         8    +1     
  Lines          166       222   +56     
  Branches        41        52   +11     
=========================================
+ Hits           166       222   +56

Impacted Files	Coverage Δ
lib/StorageError.js	`100.00% <100.00%> (ø)`
lib/remote-storage.js	`100.00% <100.00%> (ø)`
src/deploy-web.js	`100.00% <100.00%> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

sandeep-paliwal commented 1 year ago

Two things:

I agree with Jesse, + using path joins might not work as expected on windows machines?

I would advocate having all the validation as part of https://github.com/adobe/aio-cli-lib-app-config. For the following reasons:

fail fast and inform users that the config is wrong on the first command, not only on deploy

keep all the configuration validation in one place => easier to maintain. Especially knowing that we will support a json schema for the app config at some point meaning that part of the config will be handled by the schema itself.

One drawback is that if the web library is used independently we don't get the validation. But that could be mitigated in the future by exposing a validation function from the config lib.

EDIT: I understand 2. may require significant added effort, if it's easier I am also ok with keeping a note here and revisiting this once we introduce the config schema

Thanks @moritzraho . Yes I agree with part suggested by Jesse to use inbuilt path API for validation and few more extensive tests to cover windows. On the suggestion to move this to app-config, there are a few reasons the application of rules sits here

These are not config validations but application of rules present in config used at time file is being deployed. Similar to the way we use cache config and apply it to file when its deployed.
Config need not know about these business logic it should just load a valid config. But the web storage lib needs to apply valid headers based on the config rules and so this is a better place to apply the config rules. WDYT?

moritzraho commented 1 year ago

Ok then sounds good to keep the rules here then. Maybe in the future we can offload some of the validation in the schema itself.

adobe / aio-lib-web