-
Notifications
You must be signed in to change notification settings - Fork 16
Fixing pull --beta and refactoring along the way #1084
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
Changes from all commits
23e56dc
e4584ab
5e919d5
1c1ae21
0b43135
518ec0b
2eeb12a
17fe1bc
b511a8c
12502a6
c09b04e
7160782
21ac5cc
4f98667
cf572a6
b0c9090
6ea477f
87b8d18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@openfn/cli': patch | ||
| --- | ||
|
|
||
| Update Project dependency |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@openfn/project': patch | ||
| --- | ||
|
|
||
| Refactor project.repo to project.config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import { confirm } from '@inquirer/prompts'; | |
| import path from 'path'; | ||
| import fs from 'node:fs/promises'; | ||
| import { DeployConfig, getProject } from '@openfn/deploy'; | ||
| import Project from '@openfn/project'; | ||
| import Project, { Workspace } from '@openfn/project'; | ||
| import type { Logger } from '../util/logger'; | ||
| import { rimraf } from 'rimraf'; | ||
| import { Opts } from '../options'; | ||
|
|
@@ -37,47 +37,52 @@ export type PullOptionsBeta = Required< | |
| export async function handler(options: PullOptionsBeta, logger: Logger) { | ||
| const { OPENFN_API_KEY, OPENFN_ENDPOINT } = process.env; | ||
|
|
||
| const config: Partial<Config> = { | ||
| const cfg: Partial<Config> = { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This old config stuff was just a cheap way to use the old deploy API. We need to phase it out soon |
||
| apiKey: options.apiKey, | ||
| endpoint: options.endpoint, | ||
| }; | ||
|
|
||
| if (!options.apiKey && OPENFN_API_KEY) { | ||
| logger.info('Using OPENFN_API_KEY environment variable'); | ||
| config.apiKey = OPENFN_API_KEY; | ||
| cfg.apiKey = OPENFN_API_KEY; | ||
| } | ||
|
|
||
| if (!options.endpoint && OPENFN_ENDPOINT) { | ||
| logger.info('Using OPENFN_ENDPOINT environment variable'); | ||
| config.endpoint = OPENFN_ENDPOINT; | ||
| cfg.endpoint = OPENFN_ENDPOINT; | ||
| } | ||
|
|
||
| // TODO `path` or `output` ? | ||
| // I don't think I want to model this as output. deploy is really | ||
| // designed to run from the working folder | ||
| // could be projectPath or repoPath too | ||
| const outputRoot = path.resolve(options.path || '.'); | ||
|
|
||
| // TODO is outputRoot the right dir for this? | ||
| const workspace = new Workspace(outputRoot); | ||
| const config = workspace.getConfig(); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of the getter - I just want to use workspace.config here. Why make it private? |
||
|
|
||
| // download the state.json from lightning | ||
| const { data } = await getProject(config as DeployConfig, options.projectId); | ||
| const { data } = await getProject(cfg as DeployConfig, options.projectId); | ||
|
|
||
| // TODO if the user doesn't specify an env name, prompt for one | ||
| const name = options.env || 'project'; | ||
|
|
||
| const project = Project.from('state', data, { | ||
| endpoint: config.endpoint, | ||
| config, | ||
| endpoint: cfg.endpoint, | ||
| env: name, | ||
| fetched_at: new Date().toISOString(), | ||
| }); | ||
|
|
||
| // TODO `path` or `output` ? | ||
| // I don't think I want to model this as output. deploy is really | ||
| // designed to run from the working folder | ||
| // could be projectPath or repoPath too | ||
| const outputRoot = path.resolve(options.path || '.'); | ||
|
|
||
| const projectFileName = project.getIdentifier(); | ||
|
|
||
| await fs.mkdir(`${outputRoot}/.projects`, { recursive: true }); | ||
| let stateOutputPath = `${outputRoot}/.projects/${projectFileName}`; | ||
|
|
||
| const workflowsRoot = path.resolve( | ||
| outputRoot, | ||
| project.repo?.workflowRoot ?? 'workflows' | ||
| project.config.dirs.workflows ?? 'workflows' | ||
| ); | ||
| // Prompt before deleting | ||
| // TODO this is actually the wrong path | ||
|
|
@@ -96,7 +101,8 @@ export async function handler(options: PullOptionsBeta, logger: Logger) { | |
| await rimraf(workflowsRoot); | ||
|
|
||
| const state = project?.serialize('state'); | ||
| if (project.repo?.formats.project === 'yaml') { | ||
|
|
||
| if (project.config.formats.project === 'yaml') { | ||
| await fs.writeFile(`${stateOutputPath}.yaml`, state); | ||
| } else { | ||
| await fs.writeFile( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll recycle all this into a better patch note on release
This PR fixes a bug where workspace config settings are not properly respected by pull. This must be known.