Maorba/feature/db connection string #144
Conversation
…orba/feature/db-connection-string
bump pkg version
Shaharshaki2
left a comment
Shaharshaki2
left a comment
There was a problem hiding this comment.
Overall looks good,
left some comments
package.json
Outdated
| { | ||
| "name": "@mondaycom/apps-cli", | ||
| "version": "4.9.3", | ||
| "version": "4.9.4", |
There was a problem hiding this comment.
I'm gonna merge my "scanning-results" pr, so you'll probably need to change it
| if (!appId) appId = await DynamicChoicesService.chooseApp(); | ||
| const selectedRegion = await chooseRegionIfNeeded(parsedRegion, { appId }); | ||
| if (!name) name = await DynamicChoicesService.chooseSchedulerJob(appId, selectedRegion); | ||
| const jobs = await SchedulerService.listJobs(appId, selectedRegion); |
There was a problem hiding this comment.
why did you added this? don't you do the exact same thing inside the DynamicChoicesService.chooseSchedulerJob function?
There was a problem hiding this comment.
ahhh, I remember now.
the only reason we need the jobs is to validate that the name input matches an existing job, or otherwise it will end with 500.
so if the user doesnt input a job name, we are going to query for the jobs anyway, so we can return them from the inner function as well like you suggested.
BUT- if the user does input a name, we are not calling for "choose job" anyway, so we dont fetch jobs internally.
so either way, we need to fetch the job, so this change only makes sure we only do it once, not twice
| async chooseSchedulerJob(appId: AppId, region?: Region) { | ||
| const jobs = await SchedulerService.listJobs(appId, region); | ||
| async chooseSchedulerJob(appId: AppId, region?: Region, jobs?: SchedulerJob[]) { | ||
| if (!jobs) jobs = await SchedulerService.listJobs(appId, region); |
There was a problem hiding this comment.
I think you don't need to get the jobs as an argument, just keep doing it inside the function like before.
Shaharshaki2
left a comment
Shaharshaki2
left a comment
There was a problem hiding this comment.
lgtm, left comments
…orba/feature/db-connection-string
https://monday.monday.com/boards/3670992828/pulses/18380862511