-
Notifications
You must be signed in to change notification settings - Fork 160
feat: List deployments sub-command for deploy plugin #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor feedback, but looks good overall.
}; | ||
|
||
this.commands = { | ||
deploy: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does defining these commands mess with the top level core plugin on the CLI ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned about that, which is why I left out the lifecycleEvents
(I defined them at first, which caused deploy
to be triggered twice). I set the plugin to log this.commands
to see what the object looks like by default, and itame back undefined. In my manual testing, the deployment still works fine. Feel free to double check me on that though.
const resourceService = new ResourceService(this.serverless, this.options); | ||
const deployments = await resourceService.listDeployments(); | ||
let stringDeployments = "\n\nDeployments"; | ||
for (const dep of deployments) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no deployments exist consider adding a line that says "No deployments found for resource group" or similar.
src/services/resourceService.ts
Outdated
@@ -11,6 +11,11 @@ export class ResourceService extends BaseService { | |||
this.resourceClient = new ResourceManagementClient(this.credentials, this.subscriptionId); | |||
} | |||
|
|||
public async listDeployments() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a "get" operation and think we should rename to getDeployments
. The listing/writing is happening in the plugin.
35fa9cf
to
518144c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. I suggest adding one more tests for empty deployments
@@ -70,4 +72,15 @@ describe("Resource Service", () => { | |||
expect(ResourceManagementClient.prototype.resourceGroups.deleteMethod) | |||
.toBeCalledWith(resourceGroup); | |||
}); | |||
|
|||
it("lists deployments", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another to test when deployments are empty
Resolves [AB#352]