Skip to content

Commit

Permalink
feat: Add support for multi-django app SQL generation
Browse files Browse the repository at this point in the history
We've started to use multiple django applications to reduce contention
on the head of migration history. However, our migration review tooling
needs to support multiple django apps which it now does.
  • Loading branch information
markstory committed Oct 16, 2023
1 parent 5ca775d commit 21b9cf9
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 21 deletions.
39 changes: 33 additions & 6 deletions __tests__/util/getMigrationName.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,43 @@
import {getMigrationName} from '@app/util/getMigrationName';

test('handles garbage input', () => {
const [app, migration] = getMigrationName('', 'sentry');
expect(migration).toBe('');
expect(app).toBe('sentry')
});

test('gets filename without extension', () => {
const input = getMigrationName('./src/sentry/migrations/001_testing.py');
expect(input).toBe('001_testing');
const [app, migration] = getMigrationName('./src/sentry/migrations/001_testing.py', 'notused');
expect(migration).toBe('001_testing');
expect(app).toBe('sentry');
});

test('gets filename when passed only filename with extension', () => {
const input = getMigrationName('001_testing.py');
expect(input).toBe('001_testing');
const [app, migration] = getMigrationName('001_testing.py', 'sentry');
expect(migration).toBe('001_testing');
expect(app).toBe('sentry');
});

test('gets filename without extension', () => {
const [app, migration] = getMigrationName('./src/sentry/migrations/001_testing.py', 'notused');
expect(migration).toBe('001_testing');
expect(app).toBe('sentry')
});

test('gets filename with partial path', () => {
const [app, migration] = getMigrationName('migrations/001_testing.py', 'sentry');
expect(migration).toBe('001_testing');
expect(app).toBe('sentry');
});

test('gets application names from nested apps', () => {
const [app, migration] = getMigrationName('./src/sentry/feedback/migrations/001_testing.py', 'notused');
expect(migration).toBe('001_testing');
expect(app).toBe('feedback')
});

test('preserve single quotes (e.g. if used to escape)', () => {
const input = getMigrationName(`'foo/bar/001_testing.py'`);
expect(input).toBe(`'001_testing'`);
const [app, migration] = getMigrationName(`'foo/migrations/001_testing.py'`, 'notused');
expect(migration).toBe(`'001_testing'`);
expect(app).toBe('foo');
});
5 changes: 4 additions & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ inputs:
cmd:
required: false
description: "The command to run to generate SQL"
default: "sentry django sqlmigrate sentry"
default: "sentry django sqlmigrate"
run:
required: false
description: 'Action for the action to take. Possible values: null, "placeholder"'
name:
required: false
description: 'A unique name, required for multiple migrations in a Pull Request'
app_label:
required: false
description: 'A fallback app_label should one not be found in `migration`'
migration:
required: true
description: 'file name (or number) for the migration'
Expand Down
9 changes: 5 additions & 4 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ async function findBotComment(commentIntro: string) {
);
}

async function createPlaceholderComment(commentIntro: string) {
async function createPlaceholderComment(commentIntro: string): Promise<void> {
// See if comment already exists
const existingComment = await findBotComment(commentIntro);

Expand All @@ -56,6 +56,7 @@ async function run(): Promise<void> {
const runInput: string = core.getInput('run');
const command: string = core.getInput('cmd');
const name: string = core.getInput('name');
const appLabel: string = core.getInput('app_label');
const commentHeader: string = `This PR has a migration; here is the generated SQL for `;
const migrationInput: string = core.getInput('migration');
const useRawBody: string = core.getInput('useRawBody');
Expand All @@ -75,13 +76,13 @@ async function run(): Promise<void> {
}

// Transform migration input into usable name (e.g. either number or filename w/o extension)
const migrationName = getMigrationName(migrationInput);
core.debug(`Generating SQL for migration: ${migrationName} ...`);
const [djangoApp, migrationName] = getMigrationName(migrationInput, appLabel);
core.debug(`Generating SQL for migration: ${djangoApp} ${migrationName} ...`);

let output = '';
let error = '';

const exitCode = await exec(command, [migrationName], {
const exitCode = await exec(command, [djangoApp, migrationName], {
listeners: {
stdout: (data: Buffer) => {
output += data.toString();
Expand Down
42 changes: 32 additions & 10 deletions src/util/getMigrationName.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,38 @@
export function getMigrationName(name: string): string {
const pathRegex = /([']?)(.*?)([']?)$/;
type MigrationName = [appLabel: string, name: string];

const matches = name.trim().match(pathRegex);
export function getMigrationName(name: string, fallbackApp: string): MigrationName {
const parts = name.split('/');
const relevantParts = parts.slice(-3);

if (!matches) {
return '';
// Not enough paths segments to determine the app name.
if (relevantParts.length < 3) {
const migrationName = stripExtension(relevantParts.pop() ?? '');
return [fallbackApp, migrationName];
}

const [, startQuote, path, endQuote] = matches;
const migrationName = stripExtension(relevantParts.pop() ?? '');
let appName = fallbackApp;
if (relevantParts[relevantParts.length - 1] === 'migrations') {
appName = relevantParts[relevantParts.length - 2] ?? fallbackApp;
} else {
appName = relevantParts.pop() ?? fallbackApp;
}
if (appName && appName.startsWith("'")) {
appName = appName.slice(1);
}

return [appName, migrationName];
}

const quotePattern = /(.*?)([']?)$/;

function stripExtension(name: string): string {
const matches = name.trim().match(quotePattern);
if (!matches) {
return name;
}
let [, migration, quote] = matches;
migration = migration.replace('.py', '');

return `${startQuote}${path
.split('/')
.slice(-1)[0]
.replace('.py', '')}${endQuote}`;
return `${quote}${migration}${quote}`;
}

0 comments on commit 21b9cf9

Please sign in to comment.