get rid of source env.sh

[mmou]

WIP; test and merge after alpine PR gets merged

[teutat3s]

I also thought this would be a valuable change to do, after the alpine Dockerfile PR is ready.

I hesitated to do a quick PR, because I saw the test Dockerfiles need some fixing, too, they depend on the sourced env.sh ...

But this looks like a really nice starting point!

[mmou]

I hesitated to do a quick PR, because I saw the test Dockerfiles need some fixing, too, they depend on the sourced env.sh ...

@ddworken can you confirm that no changes to tests are required? it didn't appear to me that any of the test files depend on an env.sh file from docker/env.sh

[ddworken]

Yup, that is correct. For the integration tests, the test script runner sources test/env.sh and then docker-compose forwards them from there to the containers.

[teutat3s]

Ah very true, thank you.

I remember now my changes included another thing - I was trying to use only env vars in an .env file - not bash scripts - I don't know if that would be a plus for you, too. Because if so, the tests would need changes to reflect / test that new env var setup.

[mmou]

@teutat3s i don't see any .env files, or any changes to files in test from commit https://github.com/keybase/bot-sshca/commit/41ad75e5a45023abb99e7d41396a5f64cda03394 that affect how env vars are loaded into tests

[teutat3s]

@teutat3s i don't see any .env files, or any changes to files in test from commit 41ad75e that affect how env vars are loaded into tests

yeah, I left those changes local only.

EDIT: So my question is if it would be valuable to you, to move from env.sh to .env files?

[mmou]

oh i see. i changed env.sh to env.list (seemed more canonical, according to docker documentation). i did not touch the test code (anything with tests/env.sh). does that encompass the changes you are referring to? lmk if there is anything i missed.

[teutat3s]

Nice work. I thought now for the tests to test this new env.list setup, the test scripts need some more work - or am I wrong?

E.g. here

I'll have a closer look in a few hours and try to find all references of the old env.sh way in the tests.

[mmou]

I don't think that tests/env.sh is related to the (now-removed) env.sh file. It contains vars like KSSH_USERNAME, which are only used in the tests. This is supported by @ddworken's explanations above -

Yup, that is correct. For the integration tests, the test script runner sources test/env.sh and then docker-compose forwards them from there to the containers.

[teutat3s]

Understood. LGTM then ( : 👍

EDIT: My thoughts were not so much about the relation of the env.sh files in docker/ and tests/, but rather general considerations regarding testing. I thought that the tests should reflect the original setup as closely as possible, that's why I was thinking about changing the env.sh for the tests to env.list as well

[mmou]

ah i see what you're saying. tbh i haven't thought about these tests that much so far, but as far as making the tests reflect original setup as much as possible, there seems like there's some other things we might want to do at the same time:

• in addition to changing how env vars are passed into docker containers,
• also change how env vars are passed into the docker-compose file,
• also reuse as much entrypoint code as possible.

let's consider any work done there to be a separate PR. and if you already had some local changes, happy to take a look. thanks for pointing this out this consideration!