Skip to content

Conversation

@bznein
Copy link
Contributor

@bznein bznein commented Aug 26, 2020

This PR adds a WithAgentFlags function that sets up the AGENT_FLAGS environment variable from a slice of env var representing single agent startup flags.

All Submissions:

  • Have you signed our CLA?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

@bznein bznein requested a review from chatton August 26, 2020 10:08
Comment on lines 159 to 167
func WithAgentFlags(envs ...corev1.EnvVar) Modification {
return func(container *corev1.Container) {
agentParams := ""
for _, variable := range envs {
agentParams += " -" + variable.Name + " " + variable.Value
}
container.Env = envvar.MergeWithOverride(container.Env, []corev1.EnvVar{{Name: "AGENT_FLAGS", Value: agentParams}})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice implementation! I think however the containers package is not the correct place for this functionality. This specific function is now only exists for use with the agent (especially with the hard coded AGENT_FLAGS env var name)

To me this is something that we could leverage both in the community and enterprise operator, but I think it belongs in an agents or agent package, rather than container.

To me it makes more sense to have the following structure

[]customType -> []corev1.EnvVar -> container.WithEnvs(agentEnvs)

rather than including this logic in this package.

What do you think?

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Nice it looks great!

I think maybe we should add at least one or 2 basic unit tests to ensure we're getting the expected format, but after that I think we're good to merge!

@bznein bznein merged commit 3b8ea12 into master Aug 26, 2020
@bznein bznein deleted the add_containers_with_agent_flags branch August 26, 2020 12:28
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.

3 participants