-
Notifications
You must be signed in to change notification settings - Fork 208
Add feature to show piped config on web console #3673
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
Conversation
Co-authored-by: Khanh Tran <[email protected]>
khanhtc1202
left a comment
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.
Go bold 🙌
|
@Szarny Even though I approved this pull, we will not merge this until the version v0.32.1 can be released, so please be informed. |
/hold |
|
/hold cancel |
pkg/app/piped/cmd/piped/piped.go
Outdated
| func deepcopy(dst interface{}, src interface{}) error { | ||
| b, err := json.Marshal(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return json.Unmarshal(b, dst) | ||
| } |
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.
How about making it less generic?
For example, adding a Clone function to the config package.
func (c *Config) Clone() (*Config, error) {
}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 implement Clone method to *PipedSpec with this commit based on the following reasons
Confighas many fields, and there are many things to consider, such as whether it is initialized or not or what kind it is, therefore, the complexity of this PR will increase if we try to implement it in this PR.- currently, there is no need to generate clones other than
PipedSpec
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.
Got it! That is fine.
| if s.SecretManagement != nil { | ||
| s.SecretManagement.Mask() | ||
| } | ||
| } |
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 wish a function could iterate all fields to execute the Mask function on them.
But I understand it can be done like this for now. So it is ok.
|
@Szarny Just a nit comment. After that, we can merge this. Great work! |
nghialv
left a comment
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 done. Thank you.
What this PR does / why we need it:
Add feature to show piped config on Web UI with masked style
Which issue(s) this PR fixes:
Fixes #3649
Does this PR introduce a user-facing change?: