Add Base Commerce Command by thoughtassassin · Pull Request #384 · adobe/aio-cli-plugin-cloudmanager

thoughtassassin · 2021-07-14T20:51:26Z

Description

This will add the base class for all Commerce commands. The class encapsulates the logic of calling the POST endpoint to initiate the command and then poll the get endpoint while the command is running.

Related Issue

#383

Motivation and Context

Commerce CLI commands need to be integrated into the Cloud Manager Plugin as the Cloud Manager API will soon have commands for running CLI commands on Cloud Manager hosted Commerce programs.

How Has This Been Tested?

I have added the simplest command maintenance:status in my development environment that extends this base class. I wrote unit tests that match the unit tests in the app. All of the unit tests pass.

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.

codecov · 2021-07-14T20:53:17Z

Codecov Report

Merging #384 (7141df7) into main (1ce80f1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #384   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           50        52    +2     
  Lines         1217      1258   +41     
  Branches       148       151    +3     
=========================================
+ Hits          1217      1258   +41

Impacted Files	Coverage Δ
src/base-commerce-cli-command.js	`100.00% <100.00%> (ø)`
...manager/commerce/bin-magento/maintenance/status.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ce80f1...7141df7. Read the comment docs.

justinedelson · 2021-07-14T21:50:40Z

@thoughtassassin I understand the idea of adding this base command in a PR separate from the implementation of commands that actually use it, but the downside of this is that it isn't tested. Would it be possible to add a unit test that exercises this class? That test could, I suppose, be temporary and removed later.

justinedelson · 2021-07-14T22:13:21Z

+
+    while (result.status === 'RUNNING' || result.status === 'CREATING') {
+      await cli.wait(pollingInterval)
+      result = await this.callGet(sdk, programId, environmentId, commandId, true)


Same comment as above -- presumably this can throw an error. Although in this context, it's a bit stranger since the action wouldn't be stopped. I'm not sure what happens in that case.

Yes. Will be caught in subclass.

justinedelson

Summarizing my comments:

Should have a unit test (that this PR didn't impact coverage is a oddity that I'll need to investigate separately)
Missing error handling
Please revisit start/stop pairing

Also, please follow the contributing guide and squash your commits.

thoughtassassin · 2021-07-15T14:25:24Z

@thoughtassassin I understand the idea of adding this base command in a PR separate from the implementation of commands that actually use it, but the downside of this is that it isn't tested. Would it be possible to add a unit test that exercises this class? That test could, I suppose, be temporary and removed later.

Would it be okay if I just submit the code with the command that implements it? I have the test written for that already. Also, you'll be able to see how the error handling works.

justinedelson · 2021-07-15T14:34:17Z

Would it be okay if I just submit the code with the command that implements it?

Yes, this is totally fine.

justinedelson · 2021-07-15T20:58:09Z

+const { cli } = require('cli-ux')
+const commonFlags = require('../../../../../common-flags')
+
+class MaintenanceStatusCommand extends BaseCommerceCommand {


Obviously not for now, but something to consider in the future is some kind of dynamic generation of these classes since I expect future commands will be very boilerplatey. oclif/oclif#110 suggests there's a way to do this.

also adding base infrastucture for future commands

…adobe#384 Log warning that commerce CLI commands in general are not fully functional

justinedelson · 2021-07-16T10:50:56Z

@thoughtassassin I was doing one last manual test and got (the expected) error about the sdk methods not being there. As such, I think there should be a warning which I've added in 7141df7

Also made two other tweaks:

Changed the alias per your last set of changes.
Renamed the base class (AFAICT, the intention is for this base class to be specific to Commerce CLI not Commerce in general)

Please let me know if you're OK with these changes and I'll squash and merge.

thoughtassassin · 2021-07-16T14:18:43Z

@justinedelson I am good with the changes. Please proceed.

…#384 Log warning that commerce CLI commands in general are not fully functional Also includes some base infrastructure for future commands

# [2.2.0](2.1.0...2.2.0) (2021-07-16) ### Features * **commerce:** add initial commerce maintenance:status command. fixes [#384](#384) ([1a5b97b](1a5b97b))

github-actions · 2021-07-16T15:18:02Z

🎉 This issue has been resolved in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

justinedelson reviewed Jul 14, 2021

View reviewed changes

Comment thread src/base-commerce-command.js Outdated

justinedelson reviewed Jul 14, 2021

View reviewed changes

Comment thread src/base-commerce-command.js Outdated

justinedelson reviewed Jul 14, 2021

View reviewed changes

Comment thread src/base-commerce-command.js Outdated

justinedelson suggested changes Jul 14, 2021

View reviewed changes

thoughtassassin added a commit to thoughtassassin/aio-cli-plugin-cloudmanager that referenced this pull request Jul 15, 2021

feat(commerce): add commerce command. fixes adobe#384

569894d

thoughtassassin force-pushed the PAASENG-5959--Add-base-commerce-command branch from 610eaf5 to 569894d Compare July 15, 2021 15:21

thoughtassassin added a commit to thoughtassassin/aio-cli-plugin-cloudmanager that referenced this pull request Jul 15, 2021

feat(commerce): add commerce command. fixes adobe#384

e022727

thoughtassassin force-pushed the PAASENG-5959--Add-base-commerce-command branch from 569894d to e022727 Compare July 15, 2021 18:28

justinedelson mentioned this pull request Jul 15, 2021

Uncovered files are not reported #385

Closed

justinedelson reviewed Jul 15, 2021

View reviewed changes

feat(commerce): add commerce command. fixes adobe#384

1c50f89

also adding base infrastucture for future commands

thoughtassassin force-pushed the PAASENG-5959--Add-base-commerce-command branch from e022727 to 1c50f89 Compare July 15, 2021 21:12

feat(commerce): add initial commerce maintenance:status command. fixes …

7141df7

…adobe#384 Log warning that commerce CLI commands in general are not fully functional

justinedelson merged commit 766b415 into adobe:main Jul 16, 2021

github-actions Bot pushed a commit that referenced this pull request Jul 16, 2021

chore(release): 2.2.0 [skip ci]

ce6ef94

# [2.2.0](2.1.0...2.2.0) (2021-07-16) ### Features * **commerce:** add initial commerce maintenance:status command. fixes [#384](#384) ([1a5b97b](1a5b97b))

github-actions Bot added the released label Jul 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Base Commerce Command#384

Add Base Commerce Command#384
justinedelson merged 2 commits intoadobe:mainfrom
thoughtassassin:PAASENG-5959--Add-base-commerce-command

thoughtassassin commented Jul 14, 2021

Uh oh!

codecov Bot commented Jul 14, 2021 •

edited

Loading

Uh oh!

justinedelson commented Jul 14, 2021

Uh oh!

Uh oh!

Uh oh!

justinedelson Jul 14, 2021

Uh oh!

thoughtassassin Jul 15, 2021

Uh oh!

Uh oh!

justinedelson left a comment •

edited

Loading

Uh oh!

thoughtassassin commented Jul 15, 2021

Uh oh!

justinedelson commented Jul 15, 2021

Uh oh!

justinedelson Jul 15, 2021

Uh oh!

justinedelson commented Jul 16, 2021 •

edited

Loading

Uh oh!

thoughtassassin commented Jul 16, 2021

Uh oh!

github-actions Bot commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thoughtassassin commented Jul 14, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov Bot commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

justinedelson commented Jul 14, 2021

Uh oh!

Uh oh!

Uh oh!

justinedelson Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

thoughtassassin Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinedelson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thoughtassassin commented Jul 15, 2021

Uh oh!

justinedelson commented Jul 15, 2021

Uh oh!

justinedelson Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

justinedelson commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thoughtassassin commented Jul 16, 2021

Uh oh!

github-actions Bot commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 14, 2021 •

edited

Loading

justinedelson left a comment •

edited

Loading

justinedelson commented Jul 16, 2021 •

edited

Loading