-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added PodmanRunner to the codebase #903
Conversation
@ivotron can you please review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Eddie, looking great!
@ivotron I've commented out the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @edeediong! Couple of things to hammer out and this'll be done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last couple of tweaks!
src/popper/runner_host.py
Outdated
if not self._config.dry_run: | ||
pass | ||
elif not self._config.skip_pull and not step.skip_pull: | ||
log.info(f"[{step.id}] podman build {img}:{tag}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be podman pull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's true since I'm doing a podman pull below, thanks
src/popper/runner_host.py
Outdated
if build: | ||
log.info(f"[{step.id}] podman build {img}:{tag} {build_ctx_path}") | ||
if not self._config.dry_run: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a podman build
should be executed here instead of pass
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above also means that we are missing a test that ensures that a Dockerfile from a public github repository is properly build as part of the execution of a workflow. Maybe we can add this to the issue we mentioned earlier, the one regarding missing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked at DockerRunner, I meant to put a TODO:
instead of a pass. I've opened the issue and I'm gonna push changes to this effect.
No description provided.