Allow setting the name for the base container and fix longstanding bug with running quick pods #28752
Conversation
boring-cyborg
bot
added
provider:cncf-kubernetes
|
Would be nice to have unit tests which cover new functionality for avoid regression |
Yep.. they're on the way :) |
csm10495
requested review from
dstandish,
kaxil,
ashb and
XD-DENG
as code owners
csm10495
force-pushed
the
allow_rename_of_base_container
branch
from
012b099
to
6e2a2ad
Compare
csm10495
changed the title
csm10495
changed the title
|
This will also now fix #26796... since i kept hitting it in my tests :) @jedcunningham can you take a look when you get a chance? |
Some downstream machinary may require a specific name for the 'base' container
…running (when get_log=False Also add tests for setting the base container name
csm10495
force-pushed
the
allow_rename_of_base_container
branch
from
3518624
to
f1dbafc
Compare
| def await_container_completion(self, pod: V1Pod, container_name: str) -> None: | ||
| while not self.container_is_running(pod=pod, container_name=container_name): | ||
| while self.container_is_running(pod=pod, container_name=container_name): | ||
| time.sleep(1) |
There was a problem hiding this comment.
While this looks like a valid fix, it is logically unrelated to the base_container_name change. Could you split it into another PR?
There was a problem hiding this comment.
The problem is I can't get my new unit tests for base name with get_log=False to pass without fixing this too.
I guess I can split to do this fix first then that one after if you'd prefer.
There was a problem hiding this comment.
ack. will split em tomorrow.
|
Closing in favor of #28771 and then redoing this after that. |
Some downstream machinery may require a specific name for the 'base' container.
This change should be backwards compatible since the default base container name is still base.. it can just now be changed to something besides base.