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

Changes to output config.json have broken projects #15

Closed
purplecabbage opened this issue Nov 18, 2020 · 13 comments
Closed

Changes to output config.json have broken projects #15

purplecabbage opened this issue Nov 18, 2020 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@purplecabbage
Copy link
Member

We used to export the actions to config.json as

{
    "file-list": "https://[NAMESPACE].adobeioruntime.net/api/v1/web/aioappstash-0.0.1/file-list",
    "file-get": " ....
   ...
}

Now we are prefixing with the package name:

{
    "aioappstash-0.0.1/file-list": "https://[NAMESPACE].adobeioruntime.net/api/v1/web/aioappstash-0.0.1/file-list",
    "aioappstash-0.0.1/file-get": " ....
   ...
}
import actions from '../config.json'
...
actionWebInvoke(actions['file-list'], headers, params)
// this will now fail with undefined

This is the same issue reported by @surajpindoria in slack ...

@meryllblanchet
Copy link

@purplecabbage what's the workaround? Update CLI to latest, build and deploy project?

@purplecabbage
Copy link
Member Author

purplecabbage commented Nov 20, 2020

This also affects extension tooling.
We have changed the way we generate action urls to now include the packageName+packageVersion/actionName which means that the output zip file path has also changed.
Where a post-app-run/post-app-build hook would expect to be able to go and add/modify items in dist/actions/actionName.zip, that file no longer exists and is now dist/actions/packageName-packageVersion-actionName.zip

@alexkli
Copy link

alexkli commented Nov 20, 2020

@purplecabbage @meryllblanchet

Where a post-app-run/post-app-build hook would expect to be able to go and add/modify items in dist/actions/actionName.zip, that file no longer exists and is now dist/actions/packageName-packageVersion-actionName.zip

This has broken asset compute cli - in particular its test step. It builds the action before running the tests but now can't find the zip anymore as the zip filename has changed:

Build success, your app is ready to be deployed 👌

Error: Building action failed, did not create /home/runner/work/asset-compute-sdk/asset-compute-sdk/test-worker/dist/actions/worker.zip

This means all Asset Compute customers with custom workers won't be able to run tests after upgrading to the latest aio-cli. A quick test seems it is also not easy to rollback, since downgrading to aio cli 4 will still leave the updated plugins/dependencies around as it seems and the issue persists.

FWIW, right now we are calling the deploy command from within the plugin to trigger the zip build: https://github.com/adobe/aio-cli-plugin-asset-compute/blob/325c740b1984f274542fe1d273e7a8c248f2c17b/src/base-command.js#L94-L102

As mentioned before the "right" solution here would be an API like buildActionZip(...) that returns the path to the zip file, whatever it is. Would allow to drop the invoke command hack and be agnostic to filename and directory changes. Also it should have no log output since that is currently confusing as well as it's geared towards the interactive deploy step.

@tmathern
Copy link
Member

(Note also that our tests will fail for asset-compute-sdk until this is fixed, since we run tests on a test-worker/test-action using the asset compute cli. This means we can't release tested and validated bugfixes automatically currently).

@alexkli
Copy link

alexkli commented Nov 20, 2020

FWIW, this seems to have introduced the action.zip name change to include the package name: #12

@shazron
Copy link
Member

shazron commented Nov 23, 2020

@Himavanth can you comment on this issue, and the proposed additions to the API? We had to branch @adobe/aio-cli-plugin-app@5.1.1 and pin @adobe/aio-lib-runtime@1.1.0 to revert the changes and published app plugin 5.1.2: https://github.com/adobe/aio-cli-plugin-app/commits/5.1.2

@Himavanth
Copy link
Contributor

@alexkli That is a good suggestion. We do have part of this worked out in https://github.com/adobe/aio-lib-runtime/blob/master/src/build-actions.js#L149 but we haven't got to the point of letting users call this API directly. When this is done, it should serve your need.

@aiojbot
Copy link
Collaborator

aiojbot commented Nov 23, 2020

JIRA issue created: https://jira.corp.adobe.com/browse/ACNA-963

@shazron
Copy link
Member

shazron commented Nov 24, 2020

Moving to @adobe/aio-lib-runtime

@shazron shazron transferred this issue from adobe/aio-cli-plugin-app Nov 24, 2020
@shazron
Copy link
Member

shazron commented Nov 24, 2020

After discussion with the team I believe the way forward is:

  1. Branch off 1.1.0 tag, then publish 1.2.1, essentially reverting bc9a3d0 - this protects existing devs that didn't expect the change(s)
  2. On the master/main branch, publish 2.0.0 right away. This is so devs that are already relying on 1.2.0 functionality can upgrade to 2.x if they want to keep that functionality (however small the possibility) since 1.2.1 will break them again.

@shazron
Copy link
Member

shazron commented Nov 27, 2020

Two issues (breakage):

  1. web-src/src/config.json now includes the {package_name-package_version}/ prefixed to the action name as a key. It used to just have the action name.
  2. The built .zip files (in dist/actions) for the actions now include {package_name-package_version}- prefixed to the action name as the .zip filename.

Note that there is an inconsistency here, the web-src/src/config.json file has {package_name-package_version}/ (trailing slash) as a prefix, while for the .zip it has {package_name-package_version}- (trailing hyphen) as a prefix (note that this is the name of the folder it zips, so the original issue is the naming of the folder). This should be consistent with a -, I don't see a reason why they should be different.

1. Proposed fixes for the 1st issue:

All proposed fixes are for backwards compatibility. Assume the package version is 0.0.1 in the examples below.

  1. Ensure action names are unique for the whole project. Users will not be able to use the same action name for a different package. Keys in web-src/src/config.json will not have the {package_name-package_version}/ prefix.
packages:
  package_1:
    actions:
      action_1:
      action_2:
  package_2:
    actions:
      action_3:
      action_4:

converts to:

{
    "action_1": "some url",
    "action_2": "some url",
    "action_3": "some url",
    "action_4": "some url"
}
  1. All keys will have the {package_name-package_version}/ prefix EXCEPT actions from the first package in manifest.yml
packages:
  package_1:
    actions:
      action_1:
      action_2:
  package_2:
    actions:
      action_1:
      action_2:

converts to:

{
    "action_1": "some url",
    "action_2": "some url",
    "package_2-0.0.1/action_1": "some url",
    "package_2-0.0.1/action_2": "some url"
}
  1. All keys will have the {package_name-package_version}/ prefix, no exceptions. In addition, for the first package in manifest.yml, we export keys with only the action names (no prefix).
packages:
  package_1:
    actions:
      action_1:
      action_2:
  package_2:
    actions:
      action_1:
      action_2:

converts to:

{
    "package_1-0.0.1/action_1": "some url",
    "package_1-0.0.1/action_2": "some url",
    "package_2-0.0.1/action_1": "some url",
    "package_2-0.0.1/action_2": "some url",
    "action_1": "some url",
    "action_2": "some url"
}

2. Proposed fix for the 2nd issue:

Right now we have a builtList (see code) that is a result of building the actions and compressing it into a .zip. It is a JavaScript array of absolute file paths to the .zip files.

For example, if your app folder is /my-app:

packages:
  package_1:
    actions:
      action_1:
      action_2:
  package_2:
    actions:
      action_1:
      action_2:

the zip list is:

[
    "/my-app/dist/actions/package_1-0.0.1-action_1.zip",
    "/my-app/dist/actions/package_1-0.0.1-action_2.zip",
    "/my-app/dist/actions/package_2-0.0.1-action_1.zip",
    "/my-app/dist/actions/package_2-0.0.1-action_2.zip"
]

The proposed fix is to output this list as a .json file in dist/actionsfor other tools to parse. cc @alexkli

@alexkli
Copy link

alexkli commented Dec 1, 2020

Worth noting that the test framework of aio-cli-plugin-asset-compute would have to support multiple packages as well. Currently it assumes the default package with possibly multiple actions inside and expects a test folder structure like

test/asset-compute/<action-name>/

cc @jdelbick @tmathern

The proposed fix is to output this list as a .json file in dist/actionsfor other tools to parse.

I wonder if it wouldn't be easier to provide an js API as mentioned above:

  • build a single action - referenced by (package +) action name
  • returns the (absolute) action zip path as string
  • can be invoked inside the current cli without having to have a duplicate dependency on aio-lib-runtime or the like (which would quickly become out of date), to ensure it always uses the exact same build process (not sure how to do that, but maybe oclif has some tricks, or maybe there is some "dynamic" require feature in nodejs for that)

@shazron
Copy link
Member

shazron commented Dec 16, 2020

@Himavanth please see #20 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants