Skip to content
Merged
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
6 changes: 6 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,11 @@ module.exports = {
'no-constant-condition': ['error', { checkLoops: false }],
// Make sure variables marked with _ are ignored (ex. _varName)
'@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],
'@typescript-eslint/ban-ts-comment': [
'error',
{
'ts-ignore': 'allow-with-description',
},
],
},
};
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@types/async": "^3.0.1",
"@types/cli-table": "^0.3.0",
"@types/form-data": "^2.2.1",
"@types/git-url-parse": "^9.0.0",
"@types/jest": "^26.0.14",
"@types/js-yaml": "^3.11.1",
"@types/mkdirp": "^1.0.0",
Expand All @@ -57,6 +58,7 @@
"esbuild": "^0.11.6",
"eslint": "^7.2.0",
"eslint-config-prettier": "^6.11.0",
"git-url-parse": "^11.4.4",
"jest": "^26.5.3",
"js-yaml": "3.12.0",
"json-schema-to-typescript": "5.7.0",
Expand Down
19 changes: 7 additions & 12 deletions src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,15 @@ describe('validateConfiguration', () => {
test('parses minimal configuration', () => {
const data = { github: { owner: 'getsentry', repo: 'craft' } };

const projectConfig = validateConfiguration(data);

expect(projectConfig).toEqual(data);
expect(validateConfiguration(data)).toEqual(data);
});

test('fails with empty configuration', () => {
// this ensures that we do actually run the expect line in the catch block
expect.assertions(1);

try {
validateConfiguration({});
} catch (e) {
expect(e.message).toMatch(/should have required property/i);
}
Comment thread
BYK marked this conversation as resolved.
test('fails with bad configuration', () => {
expect(() => validateConfiguration({ zoom: 1 }))
.toThrowErrorMatchingInlineSnapshot(`
"Cannot parse configuration file:
data should NOT have additional properties"
`);
});
});

Expand Down
6 changes: 0 additions & 6 deletions src/artifact_providers/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ import {
BaseArtifactProvider,
RemoteArtifact,
} from '../artifact_providers/base';
import { getGlobalGithubConfig } from '../config';
import { logger as loggerRaw } from '../logger';
import { GithubGlobalConfig } from '../schemas/project_config';
import { getGithubClient } from '../utils/githubApi';
import {
detectContentType,
Expand Down Expand Up @@ -50,13 +48,9 @@ export class GithubArtifactProvider extends BaseArtifactProvider {
/** Github client */
public readonly github: Github;

/** Github repo configuration */
public readonly githubRepo: GithubGlobalConfig;

public constructor(config: ArtifactProviderConfig) {
super(config);
this.github = getGithubClient();
this.githubRepo = getGlobalGithubConfig();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/commands/artifacts_cmds/download.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ async function handlerMain(argv: ArtifactsDownloadOptions): Promise<any> {

const revision = argv.rev;

const artifactProvider = getArtifactProviderFromConfig();
const artifactProvider = await getArtifactProviderFromConfig();
if (artifactProvider instanceof NoneArtifactProvider) {
logger.warn(
`Artifact provider is disabled in the configuration, nothing to do.`
Expand Down
2 changes: 1 addition & 1 deletion src/commands/artifacts_cmds/list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const description = 'List artifacts';
async function handlerMain(argv: ArtifactsOptions): Promise<any> {
const revision = argv.rev;

const artifactProvider = getArtifactProviderFromConfig();
const artifactProvider = await getArtifactProviderFromConfig();
if (artifactProvider instanceof NoneArtifactProvider) {
logger.warn(
`Artifact provider is disabled in the configuration, nothing to do.`
Expand Down
11 changes: 6 additions & 5 deletions src/commands/prepare.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { existsSync, promises as fsPromises } from 'fs';
import { dirname, join, relative } from 'path';
import { join, relative } from 'path';
import * as shellQuote from 'shell-quote';
import simpleGit, { SimpleGit } from 'simple-git';
import { Arguments, Argv, CommandBuilder } from 'yargs';

import {
checkMinimalConfigVersion,
getConfigFilePath,
getConfigFileDir,
getConfiguration,
DEFAULT_RELEASE_BRANCH_NAME,
getGlobalGithubConfig,
} from '../config';
import { logger } from '../logger';
import { ChangelogPolicy } from '../schemas/project_config';
Expand Down Expand Up @@ -481,12 +482,12 @@ export async function releaseMain(argv: ReleaseOptions): Promise<any> {

// Get repo configuration
const config = getConfiguration();
const githubConfig = config.github;
const githubConfig = await getGlobalGithubConfig();

// Move to the directory where the config file is located
const configFileDir = dirname(getConfigFilePath());
const configFileDir = getConfigFileDir() || '.';
process.chdir(configFileDir);
logger.debug(`Working directory:`, configFileDir);
logger.debug(`Working directory:`, process.cwd());
Comment thread
BYK marked this conversation as resolved.

const newVersion = argv.newVersion;

Expand Down
14 changes: 10 additions & 4 deletions src/commands/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
getStatusProviderFromConfig,
getArtifactProviderFromConfig,
DEFAULT_RELEASE_BRANCH_NAME,
getGlobalGithubConfig,
} from '../config';
import { formatTable, logger } from '../logger';
import { GithubGlobalConfig, TargetConfig } from '../schemas/project_config';
Expand Down Expand Up @@ -238,6 +239,7 @@ async function getTargetList(
artifactProvider: BaseArtifactProvider
): Promise<BaseTarget[]> {
logger.debug('Initializing targets');
const githubRepo = await getGlobalGithubConfig();
const targetList: BaseTarget[] = [];
for (const targetConfig of targetConfigList) {
const targetClass = getTargetByName(targetConfig.name);
Expand All @@ -247,7 +249,11 @@ async function getTargetList(
continue;
}
try {
const target = new targetClass(targetConfig, artifactProvider);
const target = new targetClass(
targetConfig,
artifactProvider,
githubRepo
);
targetList.push(target);
} catch (err) {
logger.error(`Error creating target instance for ${targetDescriptor}!`);
Expand Down Expand Up @@ -456,7 +462,7 @@ export async function publishMain(argv: PublishOptions): Promise<any> {

// Get publishing configuration
const config = getConfiguration() || {};
const githubConfig = config.github;
const githubConfig = await getGlobalGithubConfig();
const githubClient = getGithubClient();

const newVersion = argv.newVersion;
Expand Down Expand Up @@ -492,8 +498,8 @@ export async function publishMain(argv: PublishOptions): Promise<any> {
}
logger.debug('Revision to publish: ', revision);

const statusProvider = getStatusProviderFromConfig();
const artifactProvider = getArtifactProviderFromConfig();
const statusProvider = await getStatusProviderFromConfig();
const artifactProvider = await getArtifactProviderFromConfig();
logger.debug(`Using "${statusProvider.constructor.name}" for status checks`);
logger.debug(`Using "${artifactProvider.constructor.name}" for artifacts`);

Expand Down
61 changes: 41 additions & 20 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { dirname, join } from 'path';

import ajv from 'ajv';
import { safeLoad } from 'js-yaml';
import GitUrlParse from 'git-url-parse';
import simpleGit from 'simple-git';

import { logger } from './logger';
import {
Expand Down Expand Up @@ -144,8 +146,8 @@ export function validateConfiguration(
/**
* Returns the parsed configuration file contents
*/
export function getConfiguration(): CraftProjectConfig {
if (_configCache) {
export function getConfiguration(clearCache = false): CraftProjectConfig {
if (!clearCache && _configCache) {
return _configCache;
}

Expand Down Expand Up @@ -225,26 +227,44 @@ export function isAfterEpoch(): boolean {
/**
* Return the parsed global Github configuration
*/
export function getGlobalGithubConfig(): GithubGlobalConfig {
let _globalGithubConfigCache: GithubGlobalConfig | null;
Comment thread
BYK marked this conversation as resolved.
export async function getGlobalGithubConfig(
clearCache = false
): Promise<GithubGlobalConfig> {
if (!clearCache && _globalGithubConfigCache !== undefined) {
if (_globalGithubConfigCache === null) {
throw new ConfigurationError(
'GitHub configuration not found in the config file and cannot be determined from Git'
);
}

return _globalGithubConfigCache;
}

// We extract global Github configuration (owner/repo) from top-level
// configuration
const repoGithubConfig = getConfiguration().github || {};
let repoGithubConfig = getConfiguration(clearCache).github || null;

if (!repoGithubConfig) {
throw new ConfigurationError(
'GitHub configuration not found in the config file'
);
}

if (!repoGithubConfig.owner) {
throw new ConfigurationError('GitHub target: owner not found');
const configDir = getConfigFileDir();
const remotes = await simpleGit(configDir).getRemotes(true);
const defaultRemote =
remotes.find(remote => remote.name === 'origin') || remotes[0];
const remoteUrl = defaultRemote
? GitUrlParse(defaultRemote.refs.push || defaultRemote.refs.fetch)
: { source: null };

if (remoteUrl.source === 'github.com') {
repoGithubConfig = {
owner: remoteUrl.owner,
repo: remoteUrl.name,
};
}
}

if (!repoGithubConfig.repo) {
throw new ConfigurationError('GitHub target: repo not found');
}
_globalGithubConfigCache = Object.freeze(repoGithubConfig);

return repoGithubConfig;
return getGlobalGithubConfig();
Comment thread
BYK marked this conversation as resolved.
}

/**
Expand All @@ -262,7 +282,7 @@ export function getGitTagPrefix(): string {
* @returns An instance of artifact provider (which may be the dummy
* NoneArtifactProvider if artifact storage is disabled).
*/
export function getArtifactProviderFromConfig(): BaseArtifactProvider {
export async function getArtifactProviderFromConfig(): Promise<BaseArtifactProvider> {
const projectConfig = getConfiguration();

let artifactProviderName = projectConfig.artifactProvider?.name;
Expand All @@ -280,10 +300,11 @@ export function getArtifactProviderFromConfig(): BaseArtifactProvider {
}
}

const githubRepo = await getGlobalGithubConfig();
const artifactProviderConfig = {
...projectConfig.artifactProvider?.config,
repoName: projectConfig.github.repo,
repoOwner: projectConfig.github.owner,
repoName: githubRepo.repo,
repoOwner: githubRepo.owner,
};

switch (artifactProviderName) {
Expand All @@ -306,9 +327,9 @@ export function getArtifactProviderFromConfig(): BaseArtifactProvider {
*
* @returns An instance of status provider
*/
export function getStatusProviderFromConfig(): BaseStatusProvider {
export async function getStatusProviderFromConfig(): Promise<BaseStatusProvider> {
const config = getConfiguration();
const githubConfig = config.github;
const githubConfig = await getGlobalGithubConfig();

const rawStatusProvider = config.statusProvider || {
config: undefined,
Expand Down
1 change: 0 additions & 1 deletion src/schemas/projectConfig.schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ const projectConfigJsonSchema = {
},
},
additionalProperties: false,
required: ['github'],

definitions: {
targetConfig: {
Expand Down
2 changes: 1 addition & 1 deletion src/schemas/project_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* Craft project-specific configuration
*/
export interface CraftProjectConfig {
github: GithubGlobalConfig;
github?: GithubGlobalConfig;
targets?: TargetConfig[];
preReleaseCommand?: string;
postReleaseCommand?: string;
Expand Down
3 changes: 2 additions & 1 deletion src/targets/__tests__/awsLambda.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ function getAwsLambdaTarget(): AwsLambdaLayerTarget {
name: 'aws-lambda-layer',
['testKey']: 'testValue',
},
new NoneArtifactProvider()
new NoneArtifactProvider(),
{ owner: 'getsentry', repo: 'craft' }
);
}

Expand Down
3 changes: 2 additions & 1 deletion src/targets/__tests__/crates.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ describe('getPublishOrder', () => {
name: 'crates',
noDevDeps: true,
},
new NoneArtifactProvider()
new NoneArtifactProvider(),
{ owner: 'getsentry', repo: 'craft' }
);

test('sorts crate packages properly', () => {
Expand Down
7 changes: 4 additions & 3 deletions src/targets/awsLambdaLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '../utils/githubApi';

import { logger as loggerRaw } from '../logger';
import { TargetConfig } from '../schemas/project_config';
import { GithubGlobalConfig, TargetConfig } from '../schemas/project_config';
import { BaseTarget } from './base';
import { BaseArtifactProvider } from '../artifact_providers/base';
import { ConfigurationError, reportError } from '../utils/errors';
Expand Down Expand Up @@ -61,9 +61,10 @@ export class AwsLambdaLayerTarget extends BaseTarget {

public constructor(
config: TargetConfig,
artifactProvider: BaseArtifactProvider
artifactProvider: BaseArtifactProvider,
githubRepo: GithubGlobalConfig
) {
super(config, artifactProvider);
super(config, artifactProvider, githubRepo);
this.github = getGithubClient();
this.awsLambdaConfig = this.getAwsLambdaConfig();
}
Expand Down
14 changes: 11 additions & 3 deletions src/targets/base.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { logger } from '../logger';
import { TargetConfig } from '../schemas/project_config';
import { GithubGlobalConfig, TargetConfig } from '../schemas/project_config';
import { FilterOptions } from '../stores/zeus';
import { stringToRegexp } from '../utils/filters';
import {
Expand All @@ -20,13 +20,17 @@ export class BaseTarget {
public readonly config: TargetConfig;
/** Artifact filtering options for the target */
public readonly filterOptions: FilterOptions;
/** Github repo configuration */
public readonly githubRepo: GithubGlobalConfig;

public constructor(
config: TargetConfig,
artifactProvider: BaseArtifactProvider
artifactProvider: BaseArtifactProvider,
githubRepo: GithubGlobalConfig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we default this to null/undefined to avoid having to modify every target, since most targets don't need this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of that as then we need to do non-null checks down the line. This will especially be pronounced in publish where we get all the targets without knowing the exact target type (as it is determined at runtime).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we? Why would we need to check for null if the only targets that use githubRepo are the ones that will actually have it in the constructor?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Even if they have it in the constructor, the type system will force us to make sure it is not null. We may be able to overcome this so I'll look into that later.

) {
this.artifactProvider = artifactProvider;
this.config = config;
this.githubRepo = githubRepo;
this.filterOptions = {};
if (this.config.includeNames) {
this.filterOptions.includeNames = stringToRegexp(
Expand All @@ -46,7 +50,11 @@ export class BaseTarget {
* @param version New version to be released
* @param revision Git commit SHA to be published
*/
public async publish(_version: string, _revision: string): Promise<void> {
public async publish(
_version: string,

_revision: string
): Promise<void> {
throw new Error('Not implemented');
return;
}
Expand Down
Loading