update generated artifacts for k8s and openshift so that env variables are loaded in a particular order #596
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Although this sucks in terms of order, having ${FOOBAR} would be against what's promised in the spec file :( https://docs.docker.com/compose/compose-file/#args having $FOOBAR would help in terms of testing to make sure environment variables work. |
|
@cdrage , I didn't get what you said |
procrypt
force-pushed
the
envs
branch
2 times, most recently
from
df33f4e
to
6e0085b
Compare
|
@procrypt Changed the variables to ${FOOBAR} from $FOOBAR. They should stay $FOOBAR |
|
@procrypt yes that should not make any difference, keeping it without brackets |
There was a problem hiding this comment.
@procrypt also the files below has envs that are map and not list so even this will be out of order
- https://github.com/kubernetes-incubator/kompose/blob/master/script/test/fixtures/keyonly-envs/env.yml#L21
- https://github.com/kubernetes-incubator/kompose/tree/master/script/test/fixtures/multiple-compose-files
- https://github.com/kubernetes-incubator/kompose/tree/master/script/test/fixtures/restart-options
Please update files in above!
|
@surajssd Updated. |
| "name": "DB_DBID", | ||
| "value": "openshift" | ||
| "name": "DB_HOST", | ||
| "value": "kubernetes" |
There was a problem hiding this comment.
The value should remain the same right? I mean the value should still be openshift right?
| @@ -255,19 +275,35 @@ | |||
| ], | |||
| "env": [ | |||
| { | |||
| "name": "MYSQL_ROOT_PASSWORD", | |||
| "value": "kubernetes" | |||
There was a problem hiding this comment.
Is this config correct? Because same env is created twice? Will this run on a k8s cluster?
Seems like something libcompose is not handling?
| MYSQL_DATABASE: $DB_NAME | ||
| MYSQL_PASSWORD: $DB_PASS | ||
| MYSQL_USER: $DB_USER | ||
| - "MYSQL_ROOT_PASSWORD=$ROOT_PASS" |
There was a problem hiding this comment.
You're still changing these values!
Leave them be please, no quotes, no editing them. They should stay the same as before.
| @@ -5,5 +5,5 @@ services: | |||
| image: "foobar" | |||
| restart: "no" | |||
| environment: | |||
| GITHUB: surajssd | |||
| - "GITHUB=surajssd" | |||
There was a problem hiding this comment.
Again.. quotes everywhere. Leave them the same as before.
|
Ping @surajssd @cdrage @surajnarwade need review. |
|
This means that we are no longer testing doceker-compose files with evn variables in map? |
|
Tests are failing :( Isn't this PR only re-organizing the env variables in the tests? I don't believe it's removing them? @kadel |
|
If I'm looking correctly this PR changes env vars. in all docker-compose files from map to array no? |
|
@kadel Yes. |
procrypt
force-pushed
the
envs
branch
2 times, most recently
from
3f7c8ca
to
da1723b
Compare
procrypt
changed the title
|
LGTM |
| - RACK_ENV=development | ||
| - SHOW=true | ||
| - SESSION_SECRET | ||
| GET_HOSTS_FROM: dns |
There was a problem hiding this comment.
why is this changed from list to map?
|
@kadel I might have dome it by mistake, updated the PR. |
|
@procrypt this change is done on basis of what code now? Also how is this sorting being done? Also if this sorting is done on basis of
See the screenshot above, like |
|
@surajssd I'm checking it. |
|
@surajssd Ping |
|
@procrypt please rebase on current master, lot of new changes have been added since morning, and also update the commit message since we are just updating the fixtures/configs and not changing the docker-compose! For me lot of tests fail, not sure why! |
|
@surajssd rebased |
|
@procrypt tests pass for me, please update the commit message since we are not changing docker-compose file? |
procrypt
changed the title
|
@surajssd Done. |
procrypt
force-pushed
the
envs
branch
2 times, most recently
from
9c5c82d
to
d4886a5
Compare
|
@procrypt but seems like it needs rebase now |
update created artifacts for kubernetes and openshift so that env variables are populated in a particular order
|
@surajssd Rebased. |
|
LGTM :) |
Fixes: #595
cc: @surajssd