-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove varlink support from podman #8400
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This helps with podman-remote's diet. After stripping binary, we see about a 33% smaller executable. |
I am not sure if I am doing something wrong, but stripped bin/podman is a lot smaller also.
|
I do not expect us branching master, but rather moving 2.2 development into a branch once release happens. |
@@ -176,12 +176,6 @@ you to manage and maintain those images and containers in a production environme | |||
familiar container cli commands. For more details, see the | |||
[Container Tools Guide](https://github.com/containers/buildah/tree/master/docs/containertools). | |||
|
|||
## Podman Legacy API (Varlink) |
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.
We should leave this in with a pointer to the last supported release
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.
Added back with a rewording.
for remote management of containers. However, this API has been deprecated by the REST API. | ||
Varlink support is in maintenance mode, and will be removed in a future release. | ||
For more details, you can see [this blog](https://podman.io/blogs/2020/01/17/podman-new-api.html). | ||
|
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 think it would be better to keep this, but rework it: Perhaps
## Podman Legacy API (Varlink)
Podman offered a [Varlink-based API](https://github.com/containers/podman/blob/master/docs/tutorials/varlink_remote_client.md)
for remote management of containers in versions prior to Podman v3.0. However, this API has now been removed upstream and in Podman v3.0.
For more details, you can see [this blog](https://podman.io/blogs/2020/01/17/podman-new-api.html) and [this blog](https://podman.io/blogs/2020/08/01/deprecate-and-remove-varlink-notice.html).
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.
reworded.
2bc3a05
to
70b6599
Compare
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 code changes LGTM. I'd be fine with adding verbiage to docs saying varlink has been removed in v3.0 and only usable for prior versions. I don't think we should remove the docs entirely though. Varlink will be limping along in RHEL 8.2 and other places longer than we'd probably like.
# Podman varlink remote-client tutorial [DEPRECATED] | ||
|
||
## What is the varlink client | ||
|
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'd keep this, but add something like:
### **NOTE:** as of Podman v3.0 the varlink client has been removed and is only usable in versions prior to Podman v3.0.
People may still use v2.0 for a while (RHEL 8.2 and earlier) and will want to get to this.
i added the WIP to the title to avoid it from being accidently merged until 2.2 is done. Also, no fair! I think about three of us wanted to do this PR! Maybe we get the kill shot with the /lgtm when it is time. |
Note to self and others, I need to send a notification to the Podman mailing list telling people of varlinks removal. I know this will plan in RHEL 8.4, Fedora wise it will be part of Fedora 34 and available to install for 32 and 33? |
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.
Small cleanups
go.mod
Outdated
@@ -57,7 +57,7 @@ require ( | |||
github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2 | |||
github.com/uber/jaeger-client-go v2.25.0+incompatible | |||
github.com/uber/jaeger-lib v2.2.0+incompatible // indirect | |||
github.com/varlink/go v0.0.0-20190502142041-0f1d566d194b | |||
github.com/varlink/go v0.4.0 |
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.
github.com/varlink/go v0.4.0 |
go.sum
Outdated
github.com/varlink/go v0.4.0 h1:+/BQoUO9eJK/+MTSHwFcJch7TMsb6N6Dqp6g0qaXXRo= | ||
github.com/varlink/go v0.4.0/go.mod h1:DKg9Y2ctoNkesREGAEak58l+jOC6JU2aqZvUYs5DynU= |
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.
github.com/varlink/go v0.4.0 h1:+/BQoUO9eJK/+MTSHwFcJch7TMsb6N6Dqp6g0qaXXRo= | |
github.com/varlink/go v0.4.0/go.mod h1:DKg9Y2ctoNkesREGAEak58l+jOC6JU2aqZvUYs5DynU= |
vendor/modules.txt
Outdated
@@ -549,9 +549,8 @@ github.com/ulikunitz/xz | |||
github.com/ulikunitz/xz/internal/hash | |||
github.com/ulikunitz/xz/internal/xlog | |||
github.com/ulikunitz/xz/lzma | |||
# github.com/varlink/go v0.0.0-20190502142041-0f1d566d194b | |||
# github.com/varlink/go v0.4.0 |
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.
# github.com/varlink/go v0.4.0 |
b771104
to
84131e2
Compare
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.
Fair well, old friend.
A quick initial glance has me as puzzled as perhaps you are - It appears you did everything right 😕 Normally Cirrus-CI is much more noisy about a bad |
...Yep, something on the Cirrus-CI end seems screwed up. The job triggered, but never did anything.. When I've seen similar "weird" behavior like this in the past, it's been due to outages/failures in Cirrus's GKE (kube) infrastructure (like nodes failing). Let me see if I can re-trigger a build via the API, but I've never done this before. The fastest thing is likely (and annoyingly) to push a change (like a rebase) to the PR. |
Well the good news is, I was able to successfully re-trigger the original build through the Cirrus-CI API. The bad news is, it didn't do anything 😆 I'll try again but with a "create" build call as opposed to a "Retrigger" call... |
Ha! it worked! |
aww dang...it almost worked. |
Signed-off-by: Daniel J Walsh <[email protected]>
@baude @mheon @vrothberg @TomSweeneyRedHat @jwhonce @ashley-cui PTAL. |
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.
LGTM
LGTM |
/lgtm |
Signed-off-by: Daniel J Walsh [email protected]