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

tester_env#250

Merged
Arnaud Lemaire (n0rad) merged 1 commit intoblablacar:masterfrom
nyodas:tester_env
Aug 18, 2017
Merged

tester_env#250
Arnaud Lemaire (n0rad) merged 1 commit intoblablacar:masterfrom
nyodas:tester_env

Conversation

@nyodas
Copy link

Matthieu "Puckel_" Roisil (@puckel)
Fix issue when we want to add environment variable to the tester only.

@nyodas
Copy link
Author

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 {

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)

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

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

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

My bad I misread the change

@n0rad Arnaud Lemaire (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