Skip to content

Conversation

@baude
Copy link
Member

@baude baude commented May 18, 2018

First pass at implement API endpoints for create and start.

Signed-off-by: baude bbaude@redhat.com

Copy link
Member

Choose a reason for hiding this comment

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

We should move all these into createConfig.GetContainerCreateOptions() so we can share code between this, run, and create

Copy link
Member Author

Choose a reason for hiding this comment

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

can we defer that to another PR; given the huge change, i really wanted to achieve getting this working first.

Copy link
Member Author

Choose a reason for hiding this comment

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

deferred

Copy link
Member

Choose a reason for hiding this comment

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

Extra whitespace

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

Instead of making BundlePath public, it would probably be better to have a ctr.ControlSocketPath() that returns this

Copy link
Member Author

Choose a reason for hiding this comment

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

i like that idea ... done

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #809) made this pull request unmergeable. Please resolve the merge conflicts.

@baude baude force-pushed the varlinkstart branch 3 times, most recently from 66f5c3c to 08acf68 Compare May 21, 2018 13:40
First pass at implement API endpoints for create and start.

Signed-off-by: baude <bbaude@redhat.com>
@mheon
Copy link
Member

mheon commented May 21, 2018

I'm going to LGTM this, but we really need to do refactoring work to clean this up soon - we now have duplicated some of the create code in three places.

@baude
Copy link
Member Author

baude commented May 21, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit f676606 has been approved by baude

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit f676606 with merge 82feafe...

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr
Approved by: baude
Pushing 82feafe to master...

@baude baude deleted the varlinkstart branch December 22, 2019 19:16
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants