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.

Sign up for GitHub

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

Jump to bottom

Make filenames defined in action config files be treated as relative to the action config file #1636

Merged
merged 3 commits into from Jan 4, 2024
Merged

Make filenames defined in action config files be treated as relative to the action config file #1636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cc67d4
Update file name to be read as absolute
Ekrekr Jan 3, 2024
8ebeb9e
Merge branch 'main_v3' into relative-file-paths
Ekrekr Jan 3, 2024
6dead30
Fix tests
Ekrekr Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 42 additions & 27 deletions core/main.ts
View file
Open in desktop
Expand Up @@ -113,17 +113,7 @@ function loadActionConfigs(session: Session, filePaths: string[]) {
)
.sort()
.forEach(actionConfigsPath => {
let actionConfigsAsJson = {};
try {
// tslint:disable-next-line: tsr-detect-non-literal-require
actionConfigsAsJson = nativeRequire(actionConfigsPath).asJson();
} catch (e) {
session.compileError(e, actionConfigsPath);
}
// TODO(ekrekr): Throw nice errors if the proto is invalid.
verifyObjectMatchesProto(dataform.ActionConfigs, actionConfigsAsJson);
const actionConfigs = dataform.ActionConfigs.fromObject(actionConfigsAsJson);

const actionConfigs = loadActionConfigsFile(session, actionConfigsPath);
actionConfigs.actions.forEach(nonProtoActionConfig => {
const actionConfig = dataform.ActionConfig.create(nonProtoActionConfig);

Expand All @@ -147,29 +137,54 @@ function loadActionConfigs(session: Session, filePaths: string[]) {
actionConfig.target.name = fileNameAsTargetName;
}

// Users define file paths relative to action config path, but internally and in the
// compiled graph they are absolute paths.
actionConfig.fileName =
actionConfigsPath.substring(0, actionConfigsPath.length - "actions.yaml".length) +
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: put actions.yaml in some constant.

Copy link
Contributor Author

@Ekrekr Ekrekr Jan 4, 2024 •
edited

Choose a reason for hiding this comment

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

Fair shout, I'll do this in a later follow up PR with all hardcoded filenames

actionConfig.fileName;

if (fileExtension === "ipynb") {
const notebookContents = nativeRequire(actionConfig.fileName).asBase64String();
session.notebook(actionConfig, notebookContents);
}

if (fileExtension === "sql") {
const queryAsContextable = nativeRequire(actionConfig.fileName).queryAsContextable;

if (actionConfig.assertion) {
return session.assertion(actionConfig, queryAsContextable);
}
if (actionConfig.table) {
return session.table(actionConfig, queryAsContextable);
}
if (actionConfig.incrementalTable) {
return session.incrementalTable(actionConfig, queryAsContextable);
}
if (actionConfig.view) {
return session.view(actionConfig, queryAsContextable);
}
// If no config is specified, the operation action type is defaulted to.
session.operate(actionConfig, queryAsContextable);
loadSqlFile(session, actionConfig);
}
});
});
}

function loadActionConfigsFile(
session: Session,
actionConfigsPath: string
): dataform.ActionConfigs {
let actionConfigsAsJson = {};
try {
// tslint:disable-next-line: tsr-detect-non-literal-require
actionConfigsAsJson = nativeRequire(actionConfigsPath).asJson();
} catch (e) {
session.compileError(e, actionConfigsPath);
}
verifyObjectMatchesProto(dataform.ActionConfigs, actionConfigsAsJson);
return dataform.ActionConfigs.fromObject(actionConfigsAsJson);
}

function loadSqlFile(session: Session, actionConfig: dataform.ActionConfig) {
const queryAsContextable = nativeRequire(actionConfig.fileName).queryAsContextable;

if (actionConfig.assertion) {
return session.assertion(actionConfig, queryAsContextable);
}
if (actionConfig.table) {
return session.table(actionConfig, queryAsContextable);
}
if (actionConfig.incrementalTable) {
return session.incrementalTable(actionConfig, queryAsContextable);
}
if (actionConfig.view) {
return session.view(actionConfig, queryAsContextable);
}
// If no config is specified, the operation action type is defaulted to.
session.operate(actionConfig, queryAsContextable);
}
26 changes: 13 additions & 13 deletions core/main_test.ts
View file
Open in desktop
Expand Up @@ -541,7 +541,7 @@ select 1 AS \${dataform.projectConfig.vars.columnVar}`
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/notebook.ipynb`
- fileName: notebook.ipynb`
);

return projectDir;
Expand Down Expand Up @@ -638,8 +638,8 @@ actions:
});
});

suite("SQL actions", () => {
test(`SQL actions can be loaded via an actions config file`, () => {
suite("action configs", () => {
test(`SQL actions can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand All @@ -653,7 +653,7 @@ actions:
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/action.sql`
- fileName: action.sql`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand Down Expand Up @@ -695,7 +695,7 @@ actions:
);
});

test(`declarations can be loaded via an actions config file`, () => {
test(`declarations can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand Down Expand Up @@ -747,7 +747,7 @@ actions:
);
});

test(`tables can be loaded via an actions config file`, () => {
test(`tables can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand All @@ -761,7 +761,7 @@ actions:
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/action.sql
- fileName: action.sql
table: {}`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
Expand Down Expand Up @@ -808,7 +808,7 @@ actions:
);
});

test(`incremental tables can be loaded via an actions config file`, () => {
test(`incremental tables can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand All @@ -822,7 +822,7 @@ actions:
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/action.sql
- fileName: action.sql
incrementalTable:
protected: true
uniqueKey:
Expand Down Expand Up @@ -878,7 +878,7 @@ actions:
);
});

test(`views can be loaded via an actions config file`, () => {
test(`views can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand All @@ -892,7 +892,7 @@ actions:
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/action.sql
- fileName: action.sql
view: {}`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
Expand Down Expand Up @@ -939,7 +939,7 @@ actions:
);
});

test(`assertions can be loaded via an actions config file`, () => {
test(`assertions can be loaded`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand All @@ -954,7 +954,7 @@ actions:
`
actions:
- assertion: {}
fileName: definitions/action.sql`
fileName: action.sql`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand Down