Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

Conversation

@nyodas
Copy link

@nyodas nyodas commented Aug 11, 2017

@puckel
Fix issue when we want to add environment variable to the tester only.

@nyodas nyodas force-pushed the tester_env branch 4 times, most recently from 18ff960 to 1f50ceb Compare August 11, 2017 18:37
@nyodas
Copy link
Author

nyodas commented Aug 11, 2017

Fixed the comment ,splited the if.

dgr/aci-test.go Outdated
}

var environment []types.EnvironmentVariable
if tmpEnv := append(aci.manifest.Tester.Aci.App.Environment, aci.manifest.Aci.App.Environment...); len(tmpEnv) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong, append should end up in same array

Copy link
Author

Choose a reason for hiding this comment

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

Fixed
even if i don't particularly agree w/h you on the same array stuff.

dgr/aci-test.go Outdated

var environment []types.EnvironmentVariable
if tmpEnv := append(aci.manifest.Tester.Aci.App.Environment, aci.manifest.Aci.App.Environment...); len(tmpEnv) != 0 {
environment = underscore.UniqBy(tmpEnv, "Name").([]types.EnvironmentVariable)
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to change 456 files to do a uniq

Copy link
Author

Choose a reason for hiding this comment

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

Vendor install + glide madness

@n0rad
Copy link
Member

n0rad commented Aug 18, 2017

I think it's still wrong, if there is no test vars, you do not push the app vars and push only an empty list

@nyodas
Copy link
Author

nyodas commented Aug 18, 2017

I don't get it.
By default aci.manifest.Tester.Aci.App.Environment and aci.manifest.Aci.App.Environment
Are typed with []types.EnvironmentVariable
Both are initialized as empty slice.
Therefore the appending of an empty slice on an empty slice shouldn't be problematic.
In the end there wasn't any env variable set up in the manifest , it stand to reason that there is none in the resulting aci.
Additionally if you append stuff to an empty slice , the slice grows with the new stuff.
I added a few more bats tests to confirm this.

@n0rad
Copy link
Member

n0rad commented Aug 18, 2017

My bad I misread the change

@n0rad n0rad merged commit c2f4034 into blablacar:master Aug 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants