Skip to content
Open
Show file tree
Hide file tree
Changes from all 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: 3 additions & 0 deletions init/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ inputs:
description: >-
Explicitly enable or disable caching of project build dependencies.
required: false
repository-owner-type:
default: ${{ github.event.repository.owner.type }}
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a reasonable solution that avoids an (additional) API call.

To check: whether repository is available for all event types we care about (especially dynamic).

Alternative ideas:

  • Query the API, but we'd want to avoid that if it incurs an extra request. For this option, ideally we'd get this out of a call we have to make anyway.
  • Check the status code of the response. If we get a 404, then it's probably because it is a user-owned repo.

required: false
outputs:
codeql-path:
description: The path of the CodeQL binary used for analysis
Expand Down
87 changes: 78 additions & 9 deletions lib/init-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 11 additions & 5 deletions src/feature-flags/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,17 @@ export async function loadPropertiesFromApi(
}
}

logger.debug("Loaded the following values for the repository properties:");
for (const [property, value] of Object.entries(properties).sort(
([nameA], [nameB]) => nameA.localeCompare(nameB),
)) {
logger.debug(` ${property}: ${value}`);
if (Object.keys(properties).length === 0) {
logger.debug("No known repository properties were found.");
} else {
logger.debug(
"Loaded the following values for the repository properties:",
);
for (const [property, value] of Object.entries(properties).sort(
([nameA], [nameB]) => nameA.localeCompare(nameB),
)) {
logger.debug(` ${property}: ${value}`);
}
}

return properties;
Expand Down
74 changes: 65 additions & 9 deletions src/init-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@ import {
makeTelemetryDiagnostic,
} from "./diagnostics";
import { EnvVar } from "./environment";
import { Feature, Features } from "./feature-flags";
import { loadPropertiesFromApi } from "./feature-flags/properties";
import { Feature, FeatureEnablement, Features } from "./feature-flags";
import {
loadPropertiesFromApi,
RepositoryProperties,
} from "./feature-flags/properties";
import {
checkInstallPython311,
checkPacksForOverlayCompatibility,
Expand All @@ -53,7 +56,7 @@ import {
OverlayBaseDatabaseDownloadStats,
OverlayDatabaseMode,
} from "./overlay-database-utils";
import { getRepositoryNwo } from "./repository";
import { getRepositoryNwo, RepositoryNwo } from "./repository";
import { ToolsSource } from "./setup-codeql";
import {
ActionName,
Expand Down Expand Up @@ -87,6 +90,8 @@ import {
checkActionVersion,
getErrorMessage,
BuildMode,
GitHubVersion,
Result,
} from "./util";
import { checkWorkflow } from "./workflow";

Expand Down Expand Up @@ -237,12 +242,12 @@ async function run(startedAt: Date) {
);

// Fetch the values of known repository properties that affect us.
const enableRepoProps = await features.getValue(
Feature.UseRepositoryProperties,
const repositoryProperties = await loadRepositoryProperties(
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Name this to indicate that it isn't a repository properties object itself, but a Result that may contain one? E.g. repositoryPropertiesResult.

repositoryNwo,
gitHubVersion,
features,
logger,
);
const repositoryProperties = enableRepoProps
? await loadPropertiesFromApi(gitHubVersion, logger, repositoryNwo)
: {};

// Create a unique identifier for this run.
const jobRunUuid = uuidV4();
Expand Down Expand Up @@ -363,10 +368,26 @@ async function run(startedAt: Date) {
githubVersion: gitHubVersion,
apiDetails,
features,
repositoryProperties,
repositoryProperties: repositoryProperties.orElse({}),
logger,
});

if (repositoryProperties.isError()) {
addDiagnostic(
config,
// Arbitrarily choose the first language. We could also choose all languages, but that
// increases the risk of misinterpreting the data.
config.languages[0],
Comment on lines +376 to +380
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We keep using this pattern; time to add a function for it so we don't have to repeat it / the comment every time?

makeTelemetryDiagnostic(
"codeql-action/repository-properties-load-failure",
"Failed to load repository properties",
{
error: getErrorMessage(repositoryProperties.value),
},
),
);
}

await checkInstallPython311(config.languages, codeql);
} catch (unwrappedError) {
const error = wrapError(unwrappedError);
Expand Down Expand Up @@ -775,6 +796,41 @@ async function run(startedAt: Date) {
);
}

async function loadRepositoryProperties(
Copy link
Member

Choose a reason for hiding this comment

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

Move this to init.ts or properties.ts, add a JSDoc, and some unit tests?

repositoryNwo: RepositoryNwo,
gitHubVersion: GitHubVersion,
features: FeatureEnablement,
logger: Logger,
): Promise<Result<RepositoryProperties, unknown>> {
const repositoryOwnerType = getOptionalInput("repository-owner-type");
if (repositoryOwnerType === "User") {
// Users cannot have repository properties, so skip the API call.
logger.debug(
"Skipping loading repository properties because the repository is owned by a user and " +
"therefore cannot have repository properties.",
);
return Result.ok({});
}

if (!(await features.getValue(Feature.UseRepositoryProperties))) {
logger.debug(
"Skipping loading repository properties because the UseRepositoryProperties feature flag is disabled.",
);
return Result.ok({});
}

try {
return Result.ok(
await loadPropertiesFromApi(gitHubVersion, logger, repositoryNwo),
);
} catch (error) {
logger.warning(
`Failed to load repository properties: ${getErrorMessage(error)}`,
);
return Result.error(error);
}
}

function getTrapCachingEnabled(): boolean {
// If the workflow specified something always respect that
const trapCaching = getOptionalInput("trap-caching");
Expand Down
30 changes: 30 additions & 0 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1292,3 +1292,33 @@ export function joinAtMost(

return array.join(separator);
}

type Success<T> = Result<T, never>;
type Failure<E> = Result<never, E>;

export class Result<T, E> {
Copy link
Member

Choose a reason for hiding this comment

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

I like this -- it's a good idea to start including errors in the types.

Would you mind adding docs and unit tests for this?

private constructor(
private readonly _ok: boolean,
public readonly value: T | E,
) {}

static ok<T>(value: T): Success<T> {
return new Result(true, value) as Success<T>;
}

static error<E>(error: E): Failure<E> {
return new Result(false, error) as Failure<E>;
}

isOk(): this is Success<T> {
return this._ok;
}

isError(): this is Failure<E> {
return !this._ok;
}

orElse(defaultValue: T): T {
return this._ok ? (this.value as T) : defaultValue;
Copy link
Member

Choose a reason for hiding this comment

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

Would TS accept isOk() instead of this._ok and narrow this.value to T so we don't need the as T?

}
}
Loading