pipecd icon indicating copy to clipboard operation
pipecd copied to clipboard

Support K8sApp by `pipectl init` in simplest way

Open t-kikuc opened this issue 1 year ago • 4 comments

What this PR does / why we need it:

Support generating the simplest app.pipecd.yaml for K8sApp by pipectl init , selecting Kustomize or Helm.

Which issue(s) this PR fixes:

Fixes #4753

Does this PR introduce a user-facing change?: no

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

t-kikuc avatar Jan 26 '24 08:01 t-kikuc

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (7b26b02) 31.02% compared to head (a0e9045) 31.39%. Report is 16 commits behind head on master.

Files Patch % Lines
pkg/app/pipectl/cmd/initialize/kubernetes.go 86.30% 16 Missing and 7 partials :warning:
pkg/app/pipectl/cmd/initialize/initialize.go 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4759      +/-   ##
==========================================
+ Coverage   31.02%   31.39%   +0.37%     
==========================================
  Files         225      226       +1     
  Lines       26257    26521     +264     
==========================================
+ Hits         8146     8327     +181     
- Misses      17460    17534      +74     
- Partials      651      660       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 26 '24 08:01 codecov[bot]

/review

t-kikuc avatar Jan 26 '24 11:01 t-kikuc

PR Analysis

Main theme

Enhancement

PR summary

This PR adds functionality to generate configuration for Kubernetes-based applications in the PipeCD project initialization command (pipectl init). Specifically, it allows the user to choose whether their application will manage its manifests using Kustomize or Helm and then prompts the user to enter the details needed for the chosen method, including support for a remote Helm chart or a local chart.

Type of PR

Enhancement

PR Feedback:

General suggestions

The implementation of Kubernetes configuration generation seems to cover both Helm and Kustomize users, which is comprehensive and user-friendly. By prompting users for specific details, it avoids potential misconfigurations and simplifies the initial setup process. The code looks clean and maintains consistent formatting and encapsulation.

Code feedback

  • relevant file: pkg/app/pipectl/cmd/initialize/kubernetes.go suggestion: Consider validation for user input during the configuration process. For strings that represent URLs, versions, or paths, ensuring that they are well-formed before proceeding can mitigate runtime issues. For instance, adding checks to ensure that chartRepository is a valid URL or helmVersion matches expected version patterns would be beneficial. (medium) relevant line: + chartRepository string

  • relevant file: pkg/app/pipectl/cmd/initialize/kubernetes.go suggestion: Typos in method names can make it difficult for others to understand and maintain the code. Correct the typo in helmImput to helmInput. This is a simple but important fix for code maintainability. (important) relevant line: +func helmImput(p prompt.Prompt) (*config.KubernetesDeploymentInput, error) {

  • relevant file: pkg/app/pipectl/cmd/initialize/initialize.go suggestion: Removing the panic and providing an implementation for the Kubernetes platform is good. However, it might be beneficial to check whether err is not nil after calling generateKubernetesConfig(p) and handle the error accordingly before proceeding. (medium) relevant line: + cfg, err = generateKubernetesConfig(p)

Security concerns:

no

The added functionality does not seem to introduce any direct security concerns. User inputs are used to generate configuration, which is less likely to be a security threat in the context of initial setup scripts. However, it is always good to validate and sanitize any user inputs to prevent potential security issues in the future.

github-actions[bot] avatar Jan 26 '24 11:01 github-actions[bot]

@ffjlabo Thank you so much! I fixed all above.

t-kikuc avatar Feb 16 '24 01:02 t-kikuc

@khanhtc1202 Would you check this? (not in a hurry)

t-kikuc avatar Feb 21 '24 06:02 t-kikuc