-
Notifications
You must be signed in to change notification settings - Fork 21
tester_env #250
tester_env #250
Conversation
18ff960 to
1f50ceb
Compare
|
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 { |
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.
this is wrong, append should end up in same array
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.
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) |
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.
you don't need to change 456 files to do a uniq
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.
Vendor install + glide madness
|
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 |
|
I don't get it. |
|
My bad I misread the change |
@puckel
Fix issue when we want to add environment variable to the tester only.