Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

docker-runtime: Use single quotes in bash env file template #176

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

cHYzZQo
Copy link
Contributor

@cHYzZQo cHYzZQo commented Sep 15, 2018

Using single quotes ensures that values are exported as literal values and not evaluated. Also escapes any single quotes in the value.

@coveralls
Copy link

coveralls commented Sep 15, 2018

Pull Request Test Coverage Report for Build 2137

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 24.532%

Totals Coverage Status
Change from base Build 2132: 0.02%
Covered Lines: 2436
Relevant Lines: 9930

💛 - Coveralls

@cHYzZQo
Copy link
Contributor Author

cHYzZQo commented Sep 15, 2018

Are tests expected to pass? I get the same lint failures on master. @sargun does master pass for you?

@sargun
Copy link
Contributor

sargun commented Sep 15, 2018 via email

@cHYzZQo
Copy link
Contributor Author

cHYzZQo commented Sep 15, 2018

Not urgent, just wanted to make sure it wasn't something i missing. Thanks

Using single quotes ensures that values are exported as
literal values and not evaluated. Also escapes any single
quotes in the value.
@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #176 into master will decrease coverage by 10.46%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #176       +/-   ##
===========================================
- Coverage   33.36%   22.89%   -10.47%     
===========================================
  Files          66       66               
  Lines        7876     7878        +2     
===========================================
- Hits         2628     1804      -824     
- Misses       4928     5861      +933     
+ Partials      320      213      -107
Impacted Files Coverage Δ
executor/runtime/docker/docker.go 8.6% <100%> (-41.82%) ⬇️
executor/mock/jobrunner.go 0% <0%> (-75.53%) ⬇️
vpc/bpfloader/bpfloader_linux.go 0% <0%> (-64.11%) ⬇️
executor/runtime/docker/lxcfs.go 0% <0%> (-62.5%) ⬇️
executor/runtime/docker/docker_linux.go 0% <0%> (-52.84%) ⬇️
executor/runtime/types/types.go 43.18% <0%> (-36.37%) ⬇️
uploader/uploader.go 26.38% <0%> (-23.62%) ⬇️
executor/drivers/titusdriver.go 68% <0%> (-16%) ⬇️
filesystems/xattr/degrading_linux.go 75% <0%> (-14.29%) ⬇️
... and 8 more

@cHYzZQo
Copy link
Contributor Author

cHYzZQo commented Sep 26, 2018

Rebased now that the linting fixes are in.

@sargun
Copy link
Contributor

sargun commented Sep 28, 2018

@cHYzZQo Are you ready for review?

@cHYzZQo
Copy link
Contributor Author

cHYzZQo commented Sep 28, 2018

Yes

@sargun
Copy link
Contributor

sargun commented Sep 29, 2018

lgtm.

@sargun
Copy link
Contributor

sargun commented Oct 2, 2018

@cHYzZQo This looks good to me, but the one thing that might be valuable is to see is '$' is in the string, and fall back to the old behaviour. I don't think anyone is doing string interpolation, but might as well shield ourselves?

@cHYzZQo
Copy link
Contributor Author

cHYzZQo commented Oct 2, 2018

one thing that might be valuable is to see is '$' is in the string, and fall back to the old behaviour.

Is that really what we want? I don't think that would have ever worked well because 1) the initial environment wouldn't do any interpolation so it makes the initial environment that entrypoint is running under and the environment after bash -l inconsistent. 2) order isn't guaranteed so it's hard to reliably reference any other user defined variable.

But if you are sure you want it i'll add it in.

@sargun sargun requested a review from rgulewich October 2, 2018 16:06
@sargun
Copy link
Contributor

sargun commented Oct 2, 2018

@cHYzZQo Makes sense. This looks good to me. @rgulewich do you want to take a look as well?

Copy link
Contributor

@rgulewich rgulewich left a comment

Choose a reason for hiding this comment

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

LGTM.

@sargun sargun merged commit 3a71951 into Netflix:master Oct 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants