Skip to content
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

refactor: create root level HelmConfig struct which applies to all HelmReleases #345

Closed
wants to merge 4 commits into from

Conversation

ahmad-ibra
Copy link
Contributor

Issue

First step into resolving

Description

In an effort to simplify the TUI prompts in validatorctl to not require lots of re-prompts for the helm configuration, we first need to refactor our helm related structs.

The proposed structure would be this:

  • ValidatorConfigSpec.HelmConfig defines the helm repository configuration that would be used to pull all plugin charts
  • ValidatorConfigSpec.Plugins has been simplifies to only specify the charts name, version, and values.

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   48.58%   48.94%   +0.35%     
==========================================
  Files          22       22              
  Lines        1274     1275       +1     
==========================================
+ Hits          619      624       +5     
+ Misses        587      584       -3     
+ Partials       68       67       -1     
Files Coverage Δ
api/v1alpha1/validatorconfig_types.go 100.00% <ø> (ø)
api/v1alpha1/zz_generated.deepcopy.go 8.52% <0.00%> (ø)
internal/controller/validatorconfig_controller.go 68.42% <73.91%> (+0.13%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39d59c9...aa6afe6. Read the comment docs.

// Name of the Helm chart.
Name string `json:"name" yaml:"name"`

// HelmConfig defines the configuration for a Helm repository.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repository refers to the name of a chart within a registry. So this should say "for a Helm registry".

Name string `json:"name" yaml:"name"`

// HelmConfig defines the configuration for a Helm repository.
type HelmConfig struct {
// Repository URL of the Helm chart.
Repository string `json:"repository" yaml:"repository"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propose to rename this Registry w/ comment: "Registry is the URL of a Helm registry"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because repository can't be configured globally - it varies for each chart.

Chart HelmChart `json:"chart" yaml:"chart"`
// Name of the Helm chart.
Name string `json:"name" yaml:"name"`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to track repository here in addition to the name:

// Repository containing the Helm chart.
Repository string `json:"repository" yaml:"repository"`

Structure of Helm URI: <registry>/<repository>/<chart>

@ahmad-ibra ahmad-ibra closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants