-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Come up with a standard to pass configuration to the deployed application #115
Conversation
src/services/k8s/knative.go
Outdated
envVarList, err := loadEnvFile(envFile) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
serviceManifest.Spec.Template.Spec.Containers[0].Env = append(serviceManifest.Spec.Template.Spec.Containers[0].Env, envVarList...) | ||
|
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 think it could be beneficial to move this in the loadManifest
function, by doing this you could also test this better by calling loadManifest with different inputs and testing the result manifest structure.
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.
moved to load manifest function.
in terms of testing I took out couple of cases that were checking load env file functionality & added sample env file as attribute to loadmanifest test case.
src/services/k8s/knative.go
Outdated
return nil, fmt.Errorf("Error loading %v file: %v", envFile, err) | ||
} | ||
} else { | ||
log.Info(fmt.Sprintf("Environment variables file %s found! Loading..", envFile)) |
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.
log.Info(fmt.Sprintf("Environment variables file %s found! Loading..", envFile)) | |
log.Infof("Environment variables file %s found! Loading..", envFile) |
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 had checked on their page for Infof and couldn't find anything related.. And then they had that for formatting messages with log.Info, that is why I went with it. Since Infof is working, switching to that one..
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.
That's interesting, should we contribute back to their repo and add the Infof
?
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 @spchortis sorry for the lengthy review cycle ;)
Closes #104