Skip to content

feat: support environment variables for local docker#1507

Closed
GlenDC wants to merge 10 commits intotestground:masterfrom
GlenDC:patch/1505-local-docker-args
Closed

feat: support environment variables for local docker#1507
GlenDC wants to merge 10 commits intotestground:masterfrom
GlenDC:patch/1505-local-docker-args

Conversation

@GlenDC
Copy link
Copy Markdown
Contributor

@GlenDC GlenDC commented Nov 2, 2022

  • Add additional_envs option to docker:runner which lets a user override envs.

Closes #1505

@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 7, 2022

@laurentsenta can this be reviewed so we can aim to merge?

@laurentsenta
Copy link
Copy Markdown
Contributor

@GlenDC 👍 Could you add tests and a line in the changelog?

@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 8, 2022

@GlenDC 👍 Could you add tests and a line in the changelog?

Sure thing. Will do and I'll let you know once it is done!

@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 9, 2022

@laurentsenta the changelog line is added, and I've added this to an existing example plan.
Would that suffice for you, if not, can you help me by specifying more exactly what more you want?

How do you actually pass run config values when running a plan via CLI? There is run-cfg but I do not see anything using it? Are there examples of this, as I suppose that would be the way to pass custom run cfgs, e.g. our custom environment variables.

@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 10, 2022

How do you actually pass run config values when running a plan via CLI? There is run-cfg but I do not see anything using it? Are there examples of this, as I suppose that would be the way to pass custom run cfgs, e.g. our custom environment variables.

Could you also explain me that @laurentsenta, as I would want to do that for my browser plan I imagine, in between different tests on same image?

glendc added 3 commits November 12, 2022 00:01
and enable additional* tests in integration tests
in the Makefile
dropped the bash-generic test as it would be too much effort
to create also the out files etc :)
@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 12, 2022

@laurentsenta all tests have been enabled and PR is ready for being merged what is me concerned.

Copy link
Copy Markdown
Contributor Author

@GlenDC GlenDC left a comment

Choose a reason for hiding this comment

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

Self-Reviewed it again as well, looks all good to me!

it wasn't enabled previously either, so I consider this out of
scope for my current task
@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 15, 2022

@laurentsenta what more would you need to be done here before we can more towards a merge path?

@laurentsenta
Copy link
Copy Markdown
Contributor

laurentsenta commented Nov 16, 2022

Feature looks fine to me thanks 👍 adding @galargh and @nonsense if they have time for a second look.

(updated description and title)

@laurentsenta laurentsenta changed the title support environment variables for local docker feat: support environment variables for local docker Nov 16, 2022
@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 18, 2022

Ok perfect. @galargh and @nonsense , do let me know if something needs to be done or how we can move this PR forward :) Thanks!

@GlenDC
Copy link
Copy Markdown
Contributor Author

GlenDC commented Nov 22, 2022

Going to close this one as for now there is not enough motivation to really add this feature, given that it's considered better practise to use parameters (SDK) for these purposes.

@GlenDC GlenDC closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support runner args for local:docker

2 participants