cdk8s-team / cdk8s-cli

Apache License 2.0
38 stars 23 forks source link

Allow overriding output directory for imports in cdk8s.yaml config file #32

Open abierbaum opened 3 years ago

abierbaum commented 3 years ago

Description of the feature or enhancement:

I think it would be useful to surface more of the configuration for the import command into the cdk8s.yaml file. This would allow users to declaratively set this in the configuration without having to put it into the command line options in a script or each time they run it.

It appears that code is already in place to do this for 'language'.

https://github.com/awslabs/cdk8s/blob/master/packages/cdk8s-cli/src/cli/cmds/import.ts#L25

This would be extending it to support output and possibly include and exclude.

Use Case:

It would help make it more convenient to use the CLI for import generation and update. It also helps to make clear through configuration what the desired settings for the project are.

Proposed Solution:

Change this line: https://github.com/awslabs/cdk8s/blob/master/packages/cdk8s-cli/src/cli/cmds/import.ts#L22

to something like: default: config.outdir ?? DEFAULT_OUTDIR

I could implement it if there is interest and likelihood of getting it through quickly.

Other:


This is a :rocket: Feature Request

eladb commented 3 years ago

Sounds good. Happy to take a contribution

abierbaum commented 3 years ago

@eladb I looked into this more and there is a complication.

Config.output is already defined here: https://github.com/awslabs/cdk8s/blob/master/packages/cdk8s-cli/src/config.ts#L15 and has a default of dist'

export interface Config {
  readonly app?: string;
  readonly language?: Language;
  readonly output?: string;
  readonly imports?: string[];
}

const DEFAULTS: Config = {
  output: 'dist',
};

The synth command relies upon this default and allows it to change in the config file to override it.

See: https://github.com/awslabs/cdk8s/blob/master/packages/cdk8s-cli/src/cli/cmds/synth.ts

So this issue is that both the import and synth commands make use of an output option but it means different things in both places, so I can't easily use it from the configuration file. We could define a new value or values in the configuration file like:

app: node index.js
language: typescript
output: dist
import_output: imports
imports:
  - k8s@1.17.0

But that seems ugly and is a pretty big change to put into a small PR.

For now I can just use a script and pass the command line option unless there is something obvious I am missing here.

pgollucci commented 3 years ago

I agree.

github-actions[bot] commented 3 years ago

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

github-actions[bot] commented 3 years ago

Closing this issue as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen.

github-actions[bot] commented 2 years ago

Closing this issue as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen.

github-actions[bot] commented 2 years ago

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

github-actions[bot] commented 2 years ago

Closing this issue as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen.

rassie commented 1 year ago

@iliapolo this would still be nice to have, could you reopen this?

github-actions[bot] commented 1 month ago

This issue has not received any attention in 1 year and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

rassie commented 1 month ago

@iliapolo Would still be nice to have. Please keep open.