spk deployment get includes PR column (if available)

[samiyaakhtar]

Adding PR column for wide output

Closes microsoft/bedrock#1167

[dennisseah]

it am not sure if having a Promise.all in a Promise.all in a good thing to do.

return new Promise((resolve, reject) => {
    Promise.all([deploymentsPromise, syncStatusesPromise])
      .then((tuple: [IDeployment[] | undefined, ITag[] | undefined]) => {
        const deployments: IDeployment[] | undefined = tuple[0];
        const syncStatuses: ITag[] | undefined = tuple[1];
        if (values.outputFormat === OUTPUT_FORMAT.WIDE) {
          getPRs(deployments);
        }
        if (values.outputFormat === OUTPUT_FORMAT.JSON) {
          console.log(JSON.stringify(deployments, null, 2));
          resolve(deployments);
        } else {
          Promise.all(promises).then(() => {
            printDeployments(
              deployments,
              values.outputFormat,
              values.nTop,
              syncStatuses
            );
            resolve(deployments);
          });
        }
      })
      .catch((e) => {
        reject(new Error(e));
      });
  });

you may want to break this block into smaller functions

[samiyaakhtar]

@dennisseah could you provide context as to why that's not a good thing to do? I'm reading online and not finding anything related. The second promise.all is needed and will only block when PR info has to be fetched, only in the case of wide output

[andrebriggs]

@dennisseah could you provide context as to why that's not a good thing to do? I'm reading online and not finding anything related. The second promise.all is needed and will only block when PR info has to be fetched, only in the case of wide output

@samiyaakhtar Breaking things down leads to a more testable design. Being able to try/catch is important for error handling. In our contributing page we highlight this:

// Async/Await based
try {
  const passwd = await promiseBasedReadFile("etc/passwd");
  console.log(passwd);
} catch (err) {
  console.error(err);
  throw err;
}```

[gemorris]

I would like to find a way to bring the "needs attention" nature of rows which are awaiting approval to the regular (non-wide) view. One way would be truncating the very wide columns trhat are already present (if this could be done meaningfully) to make room for the Merged by column (although this is not communicating the issue very strongly). Another option is to substitute the string "Awaiting Approval" or similar, for the em dash in the HLD to Manifest column. A third option would be to bring in the Status column to the CLI somehow.

[samiyaakhtar]

@gemorris @andrebriggs i think we should make the suggested changes a part of this task microsoft/bedrock#743, since this one is open and awaiting tasks under it. Perhaps we should close out this one since it adds the columns we need?

[samiyaakhtar]

Bump. Looking for some eyes on this since more improvements to this table will come as part of microsoft/bedrock#743