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

Fix for env variable. #1301

Merged
merged 2 commits into from Mar 17, 2022
Merged

Fix for env variable. #1301

merged 2 commits into from Mar 17, 2022

Conversation

enrichman
Copy link
Member

This PR fix a bug occuring during the parsing of the env variables

-> % epinio app create --env "BP_GO_BUILD_LDFLAGS=-X main.Version=myVersion" -- samplego
unable to get app configuration: Bad --env assignment `BP_GO_BUILD_LDFLAGS=-X main.Version=myVersion`, expected `name=value` as value

There was a check that an env option should be splitted in exactly two, divided by a = sign.
This was failing trying to set a variable like this VAR=foo=bar, it was not possible to set the value of foo=bar to VAR.

environment := models.EnvVariableMap{}
for _, assignment := range evAssignments {
	pieces := strings.Split(assignment, "=")
	if len(pieces) != 2 {
		return manifest, errors.New("Bad --env assignment `" + assignment + "`, expected `name=value` as value")
	}
	environment[pieces[0]] = pieces[1]
}

Moved the strings.Split to a strings.SplitN(s, sep, 2) so we can split at most at two values.

environment := models.EnvVariableMap{}
for _, assignment := range evAssignments {
	pieces := strings.SplitN(assignment, "=", 2)
	if len(pieces) < 2 {
		return manifest, errors.New("Bad --env assignment `" + assignment + "`, expected `name=value` as value")
	}
	environment[pieces[0]] = pieces[1]
}

I've also renamed the UpdateISE to UpdateICE, and refactored the UpdateICE and the UpdateBSN splitting the three logic in different functions.

note

I don't feel that we should pass the cobra.Command, the manifest package should probably just perform logic on the manifest and should not be aware of where the values are coming from, but I didn't want to change too many things.

I'm available to move that logic though. :)

@enrichman enrichman requested a review from a team as a code owner March 16, 2022 16:10
@enrichman enrichman self-assigned this Mar 16, 2022
@enrichman enrichman added kind/bug Something isn't working kind/chore Excluded from release notes. Tech dept to be addressed labels Mar 16, 2022
Copy link
Contributor

@andreas-kupries andreas-kupries left a comment

Choose a reason for hiding this comment

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

LTGM. The only nit I have is that there is no cli test demonstrating a complex --env where the changed split makes the difference. Not a blocker tough.

@enrichman enrichman merged commit deb63bc into main Mar 17, 2022
@enrichman enrichman deleted the env-option-fix branch March 17, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working kind/chore Excluded from release notes. Tech dept to be addressed
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants