Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

info: skip client-side warning about seccomp profile on API >= 1.42 #3230

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

thaJeztah
Copy link
Member

This warning will be moved to the daemon-side, similar to how it returns
other warnings. There's work in progress to change the name of the default
profile, so we may need to backport this change to prevent existing clients
from printing an incorrect warning if they're connecting to a newer daemon.

relates to moby/moby#42481

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

@justincormack @silvin-lubecki PTAL

@thaJeztah thaJeztah added this to the 21.xx milestone Aug 3, 2021
Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

I think we should still show a warning for older API versions.
Also lets make sure to hold off on this until the change is in the engine.

@thaJeztah thaJeztah changed the title info: remove client-side warning about seccomp profile info: skip client-side warning about seccomp profile on API >= 1.42 Aug 4, 2021
@thaJeztah thaJeztah force-pushed the remove_seccomp_warning branch from 3221543 to d494d4c Compare August 4, 2021 14:02
This warning will be moved to the daemon-side, similar to how it returns
other warnings. There's work in progress to change the name of the default
profile, so we may need to backport this change to prevent existing clients
from printing an incorrect warning if they're connecting to a newer daemon.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the remove_seccomp_warning branch from d494d4c to 8964595 Compare August 4, 2021 14:03
@codecov-commenter
Copy link

Codecov Report

Merging #3230 (8964595) into master (7cf5cd6) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3230      +/-   ##
==========================================
+ Coverage   58.00%   58.01%   +0.01%     
==========================================
  Files         302      302              
  Lines       21743    21753      +10     
==========================================
+ Hits        12613    12621       +8     
- Misses       8207     8208       +1     
- Partials      923      924       +1     

@@ -254,9 +255,6 @@ func prettyPrintServerInfo(dockerCli command.Cli, info types.Info) []error {
for _, o := range so.Options {
switch o.Key {
case "profile":
if o.Value != "default" {
Copy link
Member Author

@thaJeztah thaJeztah Aug 4, 2021

Choose a reason for hiding this comment

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

@cpuguy83 updated; PTAL

Alternatively, I could do:

Suggested change
if o.Value != "default" {
if o.Value != "default" && o.Value != "builtin" && versions.LessThan(dockerCli.Client().ClientVersion(), "1.42") {

Which would bring a smaller patch, but I tried to also group the warning with the other warnings (printed at the end) instead of having it "half-way" the docker info output here. Happy to change though if you think that's an issue.

@thaJeztah thaJeztah requested a review from cpuguy83 August 5, 2021 12:39
@thaJeztah
Copy link
Member Author

ping @cpuguy83 PTAL 🤗

Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants