Installer Experience v2 - Milestone 1 #2187
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer: Airgap Installer (may take a few minutes before the airgap bundle is built): Happy debugging! |
sgalsaleh
force-pushed
the
salah/sc-123874/steelthread-configure-data-dir-in-the-new
branch
from
fcd2925 to
2753604
Compare
JGAntunes
reviewed
May 26, 2025
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Missing tests for getInstallStatus and setInstallStatus.
Comment on lines
+30
to
+35
|
|
||
| func NewDiscardLogger() *logrus.Logger { | ||
| logger := logrus.New() | ||
| logger.SetOutput(io.Discard) | ||
| return logger | ||
| } |
There was a problem hiding this comment.
Thoughts on having a "debug" logger or something along those lines that writes to stdout with a well known format that allows us to distinguish between the cli logs and the API logs? This could be setup whenever the --debug flag is passed to the cli (or an env var is setup in our local dev envs for easier local dev).
JGAntunes
reviewed
May 26, 2025
Comment on lines
+18
to
+22
| type MemoryStore struct { | ||
| mu sync.RWMutex | ||
| config *types.InstallationConfig | ||
| status *types.InstallationStatus | ||
| } |
There was a problem hiding this comment.
We should add tests to this store.
Comment on lines
+10
to
+32
| func ValidateCIDR(cidr string) error { | ||
| if err := netutils.ValidateCIDR(cidr, 16, true); err != nil { | ||
| return fmt.Errorf("unable to validate cidr flag: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| type CIDRConfig struct { | ||
| PodCIDR string | ||
| ServiceCIDR string | ||
| GlobalCIDR *string | ||
| } | ||
|
|
||
| // SplitCIDR takes a CIDR string and splits it into pod and service CIDRs | ||
| // to be used for the cluster. It returns a CIDRConfig containing the split CIDRs | ||
| // and the original global CIDR. | ||
| func SplitCIDR(cidr string) (string, string, error) { | ||
| podCIDR, serviceCIDR, err := netutils.SplitNetworkCIDR(cidr) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("unable to split cidr flag: %w", err) | ||
| } | ||
| return podCIDR, serviceCIDR, nil | ||
| } |
There was a problem hiding this comment.
Should we add tests to these functions?
Comment on lines
+33
to
+65
| // GetCertificate returns a TLS certificate based on the provided configuration. | ||
| // If cert and key files are provided, it uses those. Otherwise, it generates a self-signed certificate. | ||
| func GetCertificate(cfg Config) (tls.Certificate, error) { | ||
| if cfg.CertFile != "" && cfg.KeyFile != "" { | ||
| logrus.Debugf("Using TLS configuration with cert file: %s and key file: %s", cfg.CertFile, cfg.KeyFile) | ||
| return tls.LoadX509KeyPair(cfg.CertFile, cfg.KeyFile) | ||
| } | ||
|
|
||
| hostname, altNames := generateCertHostnames(cfg.Hostname) | ||
|
|
||
| // Generate a new self-signed cert | ||
| certData, keyData, err := certutil.GenerateSelfSignedCertKey(hostname, cfg.IPAddresses, altNames) | ||
| if err != nil { | ||
| return tls.Certificate{}, fmt.Errorf("generate self-signed cert: %w", err) | ||
| } | ||
|
|
||
| cert, err := tls.X509KeyPair(certData, keyData) | ||
| if err != nil { | ||
| return tls.Certificate{}, fmt.Errorf("create TLS certificate: %w", err) | ||
| } | ||
|
|
||
| logrus.Debugf("Using self-signed TLS certificate for hostname: %s", hostname) | ||
| return cert, nil | ||
| } | ||
|
|
||
| // GetTLSConfig returns a TLS configuration with the provided certificate | ||
| func GetTLSConfig(cert tls.Certificate) *tls.Config { | ||
| return &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| CipherSuites: TLSCipherSuites, | ||
| Certificates: []tls.Certificate{cert}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we add tests to these functions?
Comment on lines
+46
to
+48
| // ListValidNetworkInterfaces returns a list of valid network interfaces that are up and not | ||
| // loopback. | ||
| func ListValidNetworkInterfaces() ([]net.Interface, error) { |
There was a problem hiding this comment.
We should test this function.
| @@ -93,3 +116,32 @@ func firstValidIPNet(i net.Interface) (*net.IPNet, error) { | |||
| } | |||
| return nil, fmt.Errorf("could not find any non-local, non podnetwork ipv4 addresses") | |||
| } | |||
|
|
|||
| func ListAllValidIPAddresses() ([]net.IP, error) { | |||
There was a problem hiding this comment.
We should test this function.
This was referenced May 27, 2025
sgalsaleh
force-pushed
the
salah/sc-123874/steelthread-configure-data-dir-in-the-new
branch
from
c83276b to
ec30def
Compare
sgalsaleh
force-pushed
the
salah/sc-123874/steelthread-configure-data-dir-in-the-new
branch
from
ec30def to
50ef28e
Compare
emosbaugh
approved these changes
May 27, 2025
sgalsaleh
deleted the
salah/sc-123874/steelthread-configure-data-dir-in-the-new
branch
This was referenced May 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Does this PR require a test?
Does this PR require a release note?
Does this PR require documentation?
NONE