feat: Add config-manager push scripts command#87
Conversation
d813027 to
4177d3d
Compare
8cd7d8c to
246e418
Compare
246e418 to
850ed52
Compare
|
Note that I rebased this PR as well, so you will need to make sure to update your local branch with these changes |
e30a786 to
4879317
Compare
4879317 to
7e67073
Compare
| CLOUD_DEPLOYMENT_TYPE_KEY, | ||
| FORGEOPS_DEPLOYMENT_TYPE_KEY, | ||
| ]; | ||
|
|
| ); | ||
|
|
||
| program | ||
| .description('Import scripts to forgeops.') |
There was a problem hiding this comment.
This is incorrect, I would just have it be Import scripts. since it works for other deployments as well
|
|
||
| const deploymentTypes = [ | ||
| CLOUD_DEPLOYMENT_TYPE_KEY, | ||
| FORGEOPS_DEPLOYMENT_TYPE_KEY, |
There was a problem hiding this comment.
I would not specify deployment type here since all deployment types support scripts since they are AM configuration
| .addOption( | ||
| new Option( | ||
| '-r, --realm <realm>', | ||
| 'Realm name, import only specified realm' | ||
| ) | ||
| ); |
| if (!(await getTokens(false, true, deploymentTypes))) { | ||
| process.exitCode = 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Small thing, but you can do this in one line:
if (!(await getTokens(false, true, deploymentTypes))) process.exit(1);| const realmsDir = getFilePath('realms/'); | ||
| const realms: string[] = realmName | ||
| ? [realmName] | ||
| : fs | ||
| .readdirSync(realmsDir, { withFileTypes: true }) | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => entry.name); |
There was a problem hiding this comment.
The root realm for forgeops gets exported to scripts, we should have it export to a directory called root because right now if we import with forgeops it will think that scripts is a realm. When reading it in, make sure root becomes / since / is the actual name of it.
| .map((entry) => entry.name); | ||
|
|
||
| for (const realm of realms) { | ||
| if (!realmName) state.setRealm(realm); |
There was a problem hiding this comment.
No need for the if check, I'd just do state.setRealm(realm)
| const { stdout } = await exec(CMD, forgeopsEnv); | ||
| expect(removeAnsiEscapeCodes(stdout)).toMatchSnapshot(); | ||
| }); | ||
| test(`"frodo config-manager push scripts -n 832807d9-fb5d-4810-88ef-6e0a1aa89924 -D ${allDirectory} -m forgeops": should import the scripts into forgeops"`, async () => { |
There was a problem hiding this comment.
The -n flag is actually not the ID, but the name of the script:
As you can see in the image, if I use ID it can't find the script, but if I use the name it can. So, update this so that -n is the script name, not the script ID.
The other thing is that you'll notice if there isn't any script change no import will be attempted. We may want to do the same thing, as scripts contain a "lastUpdated" field that indicates when an update gets made, and it updates on import whether or not the actual script value changed, so preventing the update whenever possible would make sense.
There was a problem hiding this comment.
On this same issue, once we have the library updated to support not importing changed scripts, it turns out config-manager supports with an ENV variable turning that off, so since we'll have a flag that we set on the library side to do this, we'll need a way to enable or disable that flag on the CLI side. We'll need another option for this, probably -f, --force is what I'm thinking, to force the udpate if they want to
3 participants
No description provided.