Skip to content
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: allow container re-use #67

Merged
merged 6 commits into from
Apr 9, 2022

Conversation

raddessi
Copy link
Contributor

If the spawn and cleanup commands are skipped by overriding the fixture
commands to "", None, or False then existing containers can be left
around.

Having this as a native feature would be much cleaner than how I'm currently managing this. Thank you for all the work on this project, I've used it in many others with great success :)

If the spawn and cleanup commands are skipped by overriding the fixture
commands to "", None, or False then existing containers can be left
around.
@raddessi
Copy link
Contributor Author

Ugh, I should have looked for PRs earlier, this is a dupe of #64

@Luminaar Luminaar self-requested a review March 28, 2022 12:44
@Luminaar
Copy link
Collaborator

Hello,
Thank you for your contribution. I like your PR a bit more that the older one because with your change the command can be skipped entirely. I also appreciate that you updated the readme.

I have a couple of changes I would like to make but I'm unable to push them. I think I need permissions on your fork for that. Could you please grant me those?

This was referenced Mar 28, 2022
@Luminaar
Copy link
Collaborator

Closes #63

@Luminaar
Copy link
Collaborator

Closes #54

@raddessi
Copy link
Contributor Author

Sure thing! Im on mobile right now, ill update to allow edits today. Sorry, its been a busy few days i did not see this. Thank you!

@raddessi
Copy link
Contributor Author

Huh I have this set to allow edits from you. What would you like changed?

@Luminaar
Copy link
Collaborator

Luminaar commented Apr 1, 2022

I'm not able to push to your branch for some reason. Maybe I'm doing something wrong.
I've pushed my commits to this branch. If you agree with my changes and will incorporate those commits in the PR, I'll merge it and release a new version.

@raddessi
Copy link
Contributor Author

raddessi commented Apr 1, 2022

Awesome! Im sure its fine, ill get it merged today for you. Thanks!

@Luminaar
Copy link
Collaborator

Luminaar commented Apr 7, 2022

Ping, @raddessi

@raddessi
Copy link
Contributor Author

raddessi commented Apr 7, 2022

Sorry, i see this every day i just havent got time for it. Busy week. I may today

Copy link
Contributor Author

@raddessi raddessi left a comment

Choose a reason for hiding this comment

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

Nice! Looks good to me :) Thank you and sorry for the delay

@Luminaar Luminaar merged commit 875aee3 into avast:master Apr 9, 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.

None yet

2 participants