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

feat: runs ls #640

Merged
merged 9 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
"archiver": "~7.0.1",
"axios": "~1.7.3",
"chalk": "~5.3.0",
"cli-table": "^0.3.11",
"cli-table3": "^0.6.5",
"computer-name": "~0.1.0",
"configparser": "~0.3.10",
"cors": "~2.8.5",
Expand Down Expand Up @@ -113,7 +113,6 @@
"@types/adm-zip": "^0.5.5",
"@types/archiver": "^6.0.2",
"@types/chai": "^4.3.17",
"@types/cli-table": "^0",
"@types/cors": "^2.8.17",
"@types/express": "^4.17.21",
"@types/fs-extra": "^11",
Expand Down
55 changes: 17 additions & 38 deletions src/commands/builds/ls.ts
Original file line number Diff line number Diff line change
@@ -1,37 +1,19 @@
import { Flags } from '@oclif/core';
import type { BuildCollectionClientListItem } from 'apify-client';
import chalk from 'chalk';
import Table from 'cli-table';

import { ApifyCommand } from '../../lib/apify_command.js';
import { prettyPrintStatus } from '../../lib/commands/pretty-print-status.js';
import { resolveActorContext } from '../../lib/commands/resolve-actor-context.js';
import { ResponsiveTable } from '../../lib/commands/responsive-table.js';
import { error, simpleLog } from '../../lib/outputs.js';
import { getLoggedClientOrThrow, objectGroupBy, ShortDurationFormatter } from '../../lib/utils.js';

const tableFactory = (compact = false) => {
const options: Record<string, unknown> = {
head: ['Number', 'ID', 'Status', 'Took'],
style: {
head: ['cyan', 'cyan', 'cyan', 'cyan'],
compact,
},
};

if (compact) {
options.chars = {
'mid': '',
'left-mid': '',
'mid-mid': '',
'right-mid': '',
middle: ' ',
'top-mid': '─',
'bottom-mid': '─',
};
}

return new Table<[string, string, string, string]>(options);
};
const tableFactory = () =>
new ResponsiveTable({
allColumns: ['Number', 'ID', 'Status', 'Took'],
mandatoryColumns: ['Number', 'ID', 'Status', 'Took'],
});

export class BuildLsCommand extends ApifyCommand<typeof BuildLsCommand> {
static override description = 'Lists all builds of the Actor.';
Expand Down Expand Up @@ -132,7 +114,6 @@ export class BuildLsCommand extends ApifyCommand<typeof BuildLsCommand> {
const latestBuildTag = actorInfo.versions.find((v) => v.versionNumber === actorVersion)?.buildTag;
const table = this.generateTableForActorVersion({
buildsForVersion,
compact,
buildTagToActorVersion,
});

Expand All @@ -142,7 +123,7 @@ export class BuildLsCommand extends ApifyCommand<typeof BuildLsCommand> {

const message = [
chalk.reset(`Builds for Actor Version ${chalk.yellow(actorVersion)}${latestBuildTagMessage}`),
table.toString(),
table.render(compact),
'',
];

Expand All @@ -155,15 +136,13 @@ export class BuildLsCommand extends ApifyCommand<typeof BuildLsCommand> {
}

private generateTableForActorVersion({
compact,
buildsForVersion,
buildTagToActorVersion,
}: {
compact: boolean;
buildsForVersion: BuildCollectionClientListItem[];
buildTagToActorVersion: Record<string, string>;
}) {
const table = tableFactory(compact);
const table = tableFactory();

for (const build of buildsForVersion) {
// TODO: untyped field, https://github.com/apify/apify-client-js/issues/526
Expand All @@ -173,23 +152,23 @@ export class BuildLsCommand extends ApifyCommand<typeof BuildLsCommand> {
? ` (${chalk.yellow(buildTagToActorVersion[buildNumber])})`
: '';

const tableRow: [string, string, string, string] = [
`${buildNumber}${hasTag}`,
chalk.gray(build.id),
prettyPrintStatus(build.status),
'',
];
let finishedAt: string;

if (build.finishedAt) {
const diff = build.finishedAt.getTime() - build.startedAt.getTime();

tableRow[3] = chalk.gray(`${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
finishedAt = chalk.gray(`${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
} else {
const diff = Date.now() - build.startedAt.getTime();
tableRow[3] = chalk.gray(`Running for ${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
finishedAt = chalk.gray(`Running for ${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
}

table.push(tableRow);
table.pushRow({
Number: `${buildNumber}${hasTag}`,
ID: chalk.gray(build.id),
Status: prettyPrintStatus(build.status),
Took: finishedAt,
});
}

return table;
Expand Down
135 changes: 135 additions & 0 deletions src/commands/runs/ls.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import { Flags } from '@oclif/core';
import { Timestamp } from '@sapphire/timestamp';
import chalk from 'chalk';

import { ApifyCommand } from '../../lib/apify_command.js';
import { prettyPrintStatus } from '../../lib/commands/pretty-print-status.js';
import { resolveActorContext } from '../../lib/commands/resolve-actor-context.js';
import { ResponsiveTable } from '../../lib/commands/responsive-table.js';
import { error, simpleLog } from '../../lib/outputs.js';
import { getLoggedClientOrThrow, ShortDurationFormatter } from '../../lib/utils.js';

const multilineTimestampFormatter = new Timestamp(`YYYY-MM-DD[\n]HH:mm:ss`);

const table = new ResponsiveTable({
allColumns: ['ID', 'Status', 'Results', 'Usage', 'Started At', 'Took', 'Build No.', 'Origin'],
mandatoryColumns: ['ID', 'Status', 'Results', 'Usage', 'Started At', 'Took'],
columnAlignments: {
Results: 'right',
Usage: 'right',
Took: 'right',
'Build No.': 'right',
},
});

export class RunsLsCommand extends ApifyCommand<typeof RunsLsCommand> {
static override description = 'Lists all runs of the Actor.';

static override flags = {
actor: Flags.string({
description:
'Optional Actor ID or Name to list runs for. By default, it will use the Actor from the current directory.',
}),
offset: Flags.integer({
description: 'Number of runs that will be skipped.',
default: 0,
}),
limit: Flags.integer({
description: 'Number of runs that will be listed.',
default: 10,
}),
desc: Flags.boolean({
description: 'Sort runs in descending order.',
default: false,
}),
compact: Flags.boolean({
description: 'Display a compact table.',
default: false,
char: 'c',
}),
};

static override enableJsonFlag = true;

async run() {
const { actor, desc, limit, offset, compact, json } = this.flags;

const client = await getLoggedClientOrThrow();

// TODO: technically speaking, we don't *need* an actor id to list builds. But it makes more sense to have a table of builds for a specific actor.
Copy link
Member

Choose a reason for hiding this comment

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

what does this TODO mean? maybe it should be just a regular comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been copied 1:1 from builds (where the same is applicable). We don't need an actor id passed/present to list builds/runs, as we can fetch global list, but it makes more sense to fetch per-actor. Probably best to decide for runs if that still is the case

Copy link
Member

Choose a reason for hiding this comment

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

i am actually using this page quite often, i dont think we should disallow listing all runs of a given user. i agree with builds, that feels weird without the actor context.


what i mean with my comment is that the TODO is not telling me what is missing, what should be done, what is to do, heh

const ctx = await resolveActorContext({ providedActorNameOrId: actor, client });

if (!ctx.valid) {
error({
message: `${ctx.reason}. Please run this command in an Actor directory, or specify the Actor ID by running this command with "--actor=<id>".`,
});

return;
}

const allRuns = await client.actor(ctx.id).runs().list({ desc, limit, offset });

if (json) {
return allRuns;
}

if (!allRuns.items.length) {
simpleLog({
message: 'There are no recent runs found for this Actor.',
});

return;
}

const message = [
`${chalk.reset('Showing')} ${chalk.yellow(allRuns.items.length)} out of ${chalk.yellow(allRuns.total)} runs for Actor ${chalk.yellow(ctx.userFriendlyId)} (${chalk.gray(ctx.id)})`,
];

const datasetInfos = new Map(
await Promise.all(
allRuns.items.map(async (run) =>
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 effectively N+1 problem, which is never a good thing to have in your code. let's try asking on slack if there is some way to get the data with a single API request

it's not a huge deal if there wont be too many items in the list (and we use default limit of 10, so fini'ish as long as the user doesn't try to list last 200 runs...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed on slack, we'll keep this because we can't do much at this time, and implement a spinner in the future

client
.dataset(run.defaultDatasetId)
.get()
.then(
(data) => [run.id, chalk.yellow(data?.itemCount ?? 0)] as const,
() => [run.id, chalk.gray('N/A')] as const,
),
),
),
);

for (const run of allRuns.items) {
let tookString: string;

if (run.finishedAt) {
const diff = run.finishedAt.getTime() - run.startedAt.getTime();

tookString = chalk.gray(`${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
} else {
const diff = Date.now() - run.startedAt.getTime();

tookString = chalk.gray(`Running for ${ShortDurationFormatter.format(diff, undefined, { left: '' })}`);
}

table.pushRow({
ID: chalk.gray(run.id),
Status: prettyPrintStatus(run.status),
Results: datasetInfos.get(run.id) || chalk.gray('N/A'),
Usage: chalk.cyan(`$${(run.usageTotalUsd ?? 0).toFixed(3)}`),
'Started At': multilineTimestampFormatter.display(run.startedAt),
Took: tookString,
'Build No.': run.buildNumber,
Origin: run.meta.origin ?? 'UNKNOWN',
});
}

message.push(table.render(compact));

simpleLog({
message: message.join('\n'),
});

return undefined;
}
}
79 changes: 79 additions & 0 deletions src/lib/commands/responsive-table.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import Table from 'cli-table3';

const compactModeChars = {
'mid': '',
'left-mid': '',
'mid-mid': '',
'right-mid': '',
middle: ' ',
'top-mid': '─',
'bottom-mid': '─',
};

function generateHeaderColors(length: number): string[] {
return Array.from({ length }, () => 'cyan');
}

const terminalColumns = process.stdout.columns ?? 100;

export interface ResponsiveTableOptions<
AllColumns extends string,
MandatoryColumns extends NoInfer<AllColumns> = AllColumns,
> {
/**
* Represents all the columns the that this table should show, and their order
*/
allColumns: AllColumns[];
/**
* Represents the columns that are mandatory for the user to see, even if the terminal size is less than adequate (<100).
* Make sure this field includes columns that provide enough context AND that will fit in an 80-column terminal.
*/
mandatoryColumns: MandatoryColumns[];
/**
* By default, all columns are left-aligned. You can specify columns that should be aligned in the middle or right
*/
columnAlignments?: Partial<Record<AllColumns, 'left' | 'center' | 'right'>>;
}

export class ResponsiveTable<AllColumns extends string, MandatoryColumns extends NoInfer<AllColumns> = AllColumns> {
private options: ResponsiveTableOptions<AllColumns, MandatoryColumns>;

private rows: Record<AllColumns, string>[] = [];

constructor(options: ResponsiveTableOptions<AllColumns, MandatoryColumns>) {
this.options = options;
}

pushRow(item: Record<AllColumns, string>) {
this.rows.push(item);
}

render(compact = false): string {
const head = terminalColumns < 100 ? this.options.mandatoryColumns : this.options.allColumns;
const headColors = generateHeaderColors(head.length);
const chars = compact ? compactModeChars : undefined;

const colAligns: ('left' | 'right' | 'center')[] = [];

for (const column of head) {
colAligns.push(this.options.columnAlignments?.[column] || 'left');
}

const table = new Table({
head,
style: {
head: headColors,
compact,
},
colAligns,
chars,
});

for (const rowData of this.rows) {
const row = head.map((col) => rowData[col]);
table.push(row);
}

return table.toString();
}
}
Loading
Loading