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

Additional ext reg changes - ext and ext point listing #421

Merged
merged 14 commits into from
Jul 21, 2021

Conversation

sandeep-paliwal
Copy link
Contributor

@sandeep-paliwal sandeep-paliwal commented Jun 29, 2021

Change impl prompt listing to dynamic, fetching extension point list from console
Add generic list command
Add list extension command to display implemented extensions and related details
and extension-points command to list xp available for the org
Added unit tests

Description

Related Issue

Motivation and Context

How Has This Been Tested?

manual tests

Screenshots (if appropriate):

Types of changes

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

Checklist:

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

…from console

Add generic list command
Add list extension command to display implemented extensions and related details
and extension-points command to list xp available for the org
* @param consoleCLI
*/
async function getAllExtensionPoints (consoleCLI) {
const projectConfig = aioConfig.get('project')
Copy link
Member

Choose a reason for hiding this comment

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

can you pass the aioConfig as param, it's available under the fullConfig.aio, that way we keep the loading of aioConfig inside the config-loader, that makes it easier for testing/mocking too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the function interface to accept orgId( don't need entire config anyway)

*/
async function getImplPromptChoices (consoleCLI) {
const defaultList = implPromptChoices
const projectConfig = aioConfig.get('project')
Copy link
Member

Choose a reason for hiding this comment

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

same comment here

Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

Nice, good job integrating with new changes and generator-aio-console, especially because it's all ongoing work.

Just one double minor comment, otherwise lgtm !

Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

Comments below + missing tests for init, app-helper and add extension changes

src/lib/defaults.js Show resolved Hide resolved
@@ -59,7 +58,8 @@ class AddExtensionCommand extends BaseCommand {

async selectImplementations (flags, config) {
const alreadyImplemented = config.implements
const availableChoices = implPromptChoices
const consoleCLI = await this.getLibConsoleCLI()
const availableChoices = await getImplPromptChoices(consoleCLI, config.aio)
Copy link
Member

Choose a reason for hiding this comment

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

I thought we decided that add extension and init should let the user choose extension points to implement from an hardcoded list, for two reasons:

  • there is no added benefit of adding an extension from a dynamic list for now because we anyway maintain the list in the CLI
  • dynamic list requires an extra request, slows down + more chance for bugs

Copy link
Member

Choose a reason for hiding this comment

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

@@ -108,7 +106,7 @@ class InitCommand extends BaseCommand {
// 4. retrieve workspace details, defaults to Stage
const workspace = await this.retrieveWorkspaceFromName(consoleCLI, org, project, flags.workspace)
// 5. ask for exensionPoints, only allow selection for extensions that have services enabled in Org
const extensionPoints = await this.selectExtensionPoints(flags, orgSupportedServices)
const extensionPoints = await this.selectExtensionPoints(flags, orgSupportedServices, { login: true, orgId: org.id })
Copy link
Member

Choose a reason for hiding this comment

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

In case we keep dynamic extension list, please split this into two methods:
this.selectExtensionPointsStatic(flags, orgSupportedServices)
this.selectExtensionPointsDynamic(consoleCLI, flags, orgSupportedServices, orgId)

async selectExtensionPoints (flags, orgSupportedServices = null) {
async selectExtensionPoints (flags, orgSupportedServices = null, options = {}) {
let consoleCLI
if (options.login) {
Copy link
Member

Choose a reason for hiding this comment

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

let's pass consoleCLI as input see comment above

const extConfig = this.getAppExtConfigs(flags)
const extSummary = {}

EXTENSION_POINT_LIST.forEach(extPoint => {
Copy link
Member

Choose a reason for hiding this comment

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

This is half dynamic half static.
I think in this case we can go full dynamic and get extension points too ?

// get worker impl details
if (extension.operations.apply) {
// TODO extension.manifest.full.packages[extPoint.toString()] doesnt fetch package details
const pkgDetails = extension.manifest.full.packages['dx-asset-compute-worker-1']
Copy link
Member

Choose a reason for hiding this comment

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

that is not ok, the package name can be another.. why don't you simply print all operations, no need to filter on operation names ?

Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

lgtm

@shazron shazron merged commit 8aa4d49 into release-ext-reg Jul 21, 2021
@shazron shazron deleted the additional-ext-changes branch July 21, 2021 16:17
purplecabbage added a commit that referenced this pull request Jul 29, 2021
* support single app.config.yaml and extension manifest only endpoints
* Support Multi-level config
* added support for build
* app deploy
* support for run
* support for run hooks
* no-extension for blank
* fix missing hooks
* payload string
* fix subscribe to services
* support for default legacy app gen
* fix console log
* console logs
* support for config change, application and $includes, needs changes in build/run/deploy
* fixes and support for init --no-login
* build, deploy, run with new config system
* remove aio app use --workspace flag, rename --workspace-name to --workspace
* support aio app init -w
* install improvements
* remove extensions flag, deploy to xt reg
* keep workspace-name around
* cleanup flags, introduce no- instead of skip- flags
* undeploy
* bundler fix
* fix deploy/undeploy to xt registry
* update exts names
* welcome aio app add action
* app delete
* add/delete web assets
* log web assets delete
* add and delete events
* wrap up app/delete commands
* fix stage cdn url
* expose gens via index.js
* fix init --no-extensions
* support for relative path to config
* better action, web-src default path
* fix run vscode config input
* fix run local
* support relative paths
* absRoot == path.resolve
* load as absolut path in runtime manifest
* add op[view] metadata for dx/excshell/1
* fix undeploy --no-actions
* fix undeploy + use fresh services for metadata
* fix removed util
* fix publish/unpublish
* some logs + bin for aio-next
* mocks on the way + fix backward compatibility for legacy apps
* chalk.bold new exte point log
* dx experience cloud rename template name
* fixture apps and mocks v1
* mocks done + one is tested
* mock include indexes, add tests for config, fix some mocks
* config loader 1 test case per mock app
* tests for app info + show how to use mocks
* add preid to .npmrc
* add unit-tests npm script
* removed posttest npm script (added lint script to test npm script)
* Mock rewrite utility improvements, more config loader tests (#422)
* improved mock rewrite config utility, more tests for config loader
* attempt to fix windows tests
* config-loader 100%
* fixture module names
* path.resolve can you save me ?
* feat: add `aio app test` for extension registry changes
* worker opcode change + tests BaseCommand & Init  (#424)
* More tests: BaseCommand & Init
* worker => apply
* init 100 coverage
* fix win tests
* coverage
* App helper tests + fix config loader once (#427)
* App helpers test + fix config-loader tests
* 100% cov on app helper
* add tests for app: run, build, deploy, and undeploy (Extension Registry release) (#426)
* delete ci - test coverage 100%
* delete event - test coverage 100%
* delete web-assets - test coverage 100%, fixed logic bugs for noFrontend, and user declines
* `aio app list` additions - ext and ext point listing (#421)
* Change impl prompt listing to dynamic, fetching extension point list from console
* Add generic list command
* Add list extension command to display implemented extensions and related details and extension-points command to list xp available for the org
* Add review comments, Fix ext listing based on o/p changes from console API
* Fix extension list processing
* improve o/p for extension point listing
* Fix issue with aio app init, Added ext point filtering to stop repeated from showing up
* Fix fetching org id in case of import
* Handle --no-login case for aio app init
* Fixed extension list command with new asset-compute xp operation (apply)
* Added unit tests for
-- generic List command
-- Extension-point List command
-- Fixed linting
* Added unit tests for list extension command
* Change o/p of extension list to show only impl and not src from package
* fix get-url and logs (#432)
* fixed unit tests and coverage for lib files (Ext Reg support)  (#431)
* Unit tests with 100% codecov for all app add cmds (#429)
* remove deprecation notice for manifest (#434)
* Fixes: app logs, deploy no-login legacy+app, no-build, no extra update extpoint call (#437)
* fixes: app logs
* fixes: deploy no-login legacy+app, no-build, no extra remove call
* Delete test coverage (#438)
* delete:extension tests and coverage, fix: returns error if prompt returns false
* delete action - test coverage
* fix: path.dirname mock - replace with spyOn
* fix: app/use unit tests
* fix: app/get-url unit tests
* app/get-url - remove unused dep
* fix: app/logs unit tests (and implementation)
* fix: linter issues
* fix: attempt to fix phantom CI failure (1 unit test failure left)
* rename nui operation apply => workerProcess (#439)
* fix: ci unit test issue for app/delete/event
* fix: failing unit tests on Windows (#441)
* fix: app/delete/action Windows unit tests
* fix: app/add/web-assets Windows unit tests
* fix: lib/vscode Windows unit tests

Co-authored-by: Jesse MacFadyen <purplecabbage@gmail.com>
Co-authored-by: Shazron Abdullah <shaz@adobe.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: sandeep-paliwal <spaliwal@adobe.com>
Co-authored-by: Himavanth <contacthima@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants