-
Notifications
You must be signed in to change notification settings - Fork 203
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
(feat) O3-3443 Command in the OpenMRS CLI to package up configurations #1052
base: main
Are you sure you want to change the base?
Conversation
@wikumChamith does this look to you like what @ibacher had in mind for https://openmrs.atlassian.net/browse/O3-3443? |
No, it does not. I think this does something else. @suubi-joshua How can I test this? What does it do? Why is there anything about config schemas in this? The goal of the ticket was solely to allow a user to specify some JSON files and merge those into a single thing. |
Thanks for the clarity @ibacher @wikumChamith . The script i have provided allows user to specify directories pointing to ts files containing custom configSchemas then merges them into one file. I assumed that configs are an object format in typescript format just like here configschemas not json files but will change this to read json files. When working proper this script writes the combined configs as an object in a ts file (working on testing this in cli). I am working on the testing it in the CLI. |
packages/framework/esm-framework/docs/interfaces/WorkspaceContainerProps.md
Outdated
Show resolved
Hide resolved
packages/framework/esm-framework/docs/interfaces/WorkspaceContainerProps.md
Outdated
Show resolved
Hide resolved
packages/framework/esm-framework/docs/interfaces/WorkspaceContainerProps.md
Outdated
Show resolved
Hide resolved
@ibacher @wikumChamith With testing the command to use in the cli is |
@suubi-joshua I think that it's preferable that this all gets wrapped up into the "assemble" command. I don't not want to add more steps that implementers need to run, just more options to the steps they can run. |
Also, it's better to allow users to specify files rather than directories. We can always make |
Similarly, the output file name shouldn't be required to be specified, but have a sane default. |
(Removed Wikum as he explained to me this is outside his familiarity area at this time.) |
@@ -17,6 +17,7 @@ export interface AssembleArgs { | |||
target: string; | |||
mode: string; | |||
config: Array<string>; | |||
myConfigs: Array<string>; |
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.
Better naming suggestion:
myConfigs: Array<string>; | |
configFiles: Array<string>; |
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.
Okay will work on this
@@ -303,5 +331,9 @@ export async function runAssemble(args: AssembleArgs) { | |||
await writeFile(resolve(args.target, 'spa-assemble-config.json'), JSON.stringify(versionManifest), 'utf8'); | |||
} | |||
|
|||
if(args.mode === 'myConfigs'){ | |||
await writeFile(resolve(args.target, 'My-Merged-configs.json'), JSON.stringify(config), 'utf8'); |
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.
This filename should be spa-config.json
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.
Okay
default: 'survey', | ||
description: | ||
'The source of the frontend modules to assemble. `config` uses a configuration file specified via `--config`. `survey` starts an interactive command-line survey.', | ||
'The source of the frontend modules to assemble. `config` uses a configuration file specified via `--config`. `configFiles` packages up configurations from file specified via `--config-file`. `survey` starts an interactive command-line survey.', | ||
type: 'string', | ||
}), | ||
(args) => runCommand('runAssemble', args), |
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.
Hello @ibacher I am facing a blocker. After making the changes I want to test them but there is a persistent error in cli.ts
file on the args,
somehow the new argument of configFiles
is missing from the args
in the cli
yet its exported in the AssembleArgs interface
. Also when I edit the mode option to put another choice ie configFiles
it does not seem to get added (How do I know this, when i run yarn assemble --help
my edits for choices dont show even for the description)
8f994a6
to
9cee659
Compare
Requirements
feat
,fix
, orchore
, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This script automatically merges configuration files from multiple directories into a single file. It simplifies managing app-specific configurations and reduces manual maintenance.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-3443
Other