-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add piped config-data flag #2398
Conversation
@@ -497,6 +499,14 @@ func (p *piped) loadConfig(ctx context.Context) (*config.PipedSpec, error) { | |||
return extract(cfg) | |||
} | |||
|
|||
if p.configData != "" { | |||
cfg, err := config.DecodeYAML([]byte(p.configData)) |
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.
As I can see the PR body, it's expected to be JSON. In that case, doesn't it have to use MarsharJSON or something like that?
--config-data='{"apiVersion": "pipecd.dev/v1beta1", "kind": "Piped",...}'
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.
Alternatively, we can support a kind of like --config-from-stdin
.
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.
As I can see the PR body, it's expected to be JSON. In that case, doesn't it have to use MarsharJSON or something like that?
--config-data='{"apiVersion": "pipecd.dev/v1beta1", "kind": "Piped",...}'
Well, the fact is our config.DecodeYAML
use yaml.YAMLToJSON
which allow both YAML and JSON as it input. So basically we don't need to use MarsharJSON or something like that here. In case this --config-data
flag is used, I think users would happier to pass string value in JSON format here rather than just YAML format (as we know, yaml requires tabs/spaces strictly so inline command with YAML format is not a good idea), and in the scope of writing down some samples, I chose JSON as it's easier to write.
ref: https://github.com/pipe-cd/pipe/blob/master/pkg/config/config.go#L217
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.
Alternatively, we can support a kind of like
--config-from-stdin
.
Could you provide samples for that flag, I think this --config-data
could handle most of the cases we want already 🤔
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.
Well, the fact is our config.DecodeYAML use yaml.YAMLToJSON which allow both YAML and JSON as it input.
Alright, I didn't even know that 👍
Could you provide samples for that flag,
In that case, --config-from-stdin
has no sense. Never mind.
/lgtm |
Nice add. |
What this PR does / why we need it:
Sample command would look like
or
or
Which issue(s) this PR fixes:
Fixes #2392
Does this PR introduce a user-facing change?: