-
Notifications
You must be signed in to change notification settings - Fork 207
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
Timmy/installer #4930
base: master
Are you sure you want to change the base?
Timmy/installer #4930
Conversation
d4f2f5d
to
f3a5802
Compare
86f7f2a
to
dccc23c
Compare
c89657f
to
3620dca
Compare
a3f1dcf
to
67f08f7
Compare
6752a69
to
deef18d
Compare
bd43692
to
ada346e
Compare
ada346e
to
5380d76
Compare
51d3c0c
to
624b2b9
Compare
77172ce
to
36af128
Compare
systemDrive = "C:" | ||
} | ||
script := string(cse) | ||
script = strings.ReplaceAll(script, "%SYSTEMDRIVE%", systemDrive) |
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.
You can check https://pkg.go.dev/os#ExpandEnv for an inspiration (seems like it's not windows friendly).
Do you want to envsubst a single variable or all of them?
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.
Just the one needed to be substituted. Will have a look.
if config.AgentPoolProfile.IsWindows() { | ||
customData, err2 := OldCustomData(ctx, config) | ||
if err2 != nil { | ||
log.Fatal("error:", err2) |
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.
Avoid using log.Fatal unless absolutely necessary. It exists application immediately, without calling any defer
calls or error handling.
We're using different logger.
Also, err2
?
UseAzureMsiDirectly BootstrappingMethod = "UseAzureMsiDirectly" | ||
UseSecureTLSBootstrapping BootstrappingMethod = "UseSecureTLSBootstrapping" | ||
//nolint:gosec // this is a const string to use in switch statements, not hardcoded credentials | ||
UseTLSBootstrapToken BootstrappingMethod = "UseTLSBootstrapToken" |
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 would do something like
const (
BootstrappingMethodArcMSI BootstrappingMethod = "ArcMSI"
BootstrappingMethodArcMSIToMakeCSR BootstrappingMethod = "ArcMSIToMakeCSR"
BootstrappingMethodAzureMSI BootstrappingMethod = "AzureMSI"
BootstrappingMethodAzureMSIToMakeCSR BootstrappingMethod = "AzureMSIToMakeCSR"
BootstrappingMethodSecureTLSBootstrapping BootstrappingMethod = "SecureTLSBootstrapping"
//nolint:gosec // this is a const string to use in switch statements, not hardcoded credentials
BootstrappingMethodTLSBootstrapToken BootstrappingMethod = "TLSBootstrapToken"
)
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.
It's probably not BootstrappingMethod
, but something like ClusterAuthMethod
or KuberentesAuthMethod
?
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds the following functions to node-bootstrapper:
To produce variants of kubeconfig, it introduces new config params:
The other config params relevant to bootstrapping are:
so the new one (being an enum) gives us extensibility in future and is in the same config place as the existing one.
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: