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

Support for manifests using docker experimental cli commands #622

Conversation

dims
Copy link
Member

@dims dims commented Sep 5, 2018

Re-proposing changes from #516 that were reverted in #620

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2018
@dims
Copy link
Member Author

dims commented Sep 5, 2018

cc @mkumatag @dougm @tpepper

@dims
Copy link
Member Author

dims commented Sep 5, 2018

/assign @calebamiles

@dims
Copy link
Member Author

dims commented Sep 5, 2018

The environment variable will ensure that if config.json is over-written we still will have the experimental capability in the CLI

See docker/cli#1138 on where this env variable support was added

# As of 2018-04-10, leaving this pinned to 17.09 due to image pull issues
# with 17.12
ARG DOCKER_VERSION=17.09.0~ce-0~ubuntu
ARG DOCKER_VERSION=18.06.0~ce~3-0~ubuntu
Copy link
Member

Choose a reason for hiding this comment

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

18.06.1~ce~3-0~ubuntu is already out, just see if you can use that version..

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkumatag we kinda tested the 18.06.0 bump yesterday so let's leave it alone unless 18.06.1 is absolutely necessary

Copy link
Member

Choose a reason for hiding this comment

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

this same pinning is in place in test-infra for docker in docker building (we talked to releng about this) because we started flaking a ton on image pull around then. we've also not upgraded.. filing a bug for us to upgrade as well

@mkumatag
Copy link
Member

mkumatag commented Sep 5, 2018

I tested docker manifest --help command it worked after setting that environment variable

root@localhost:~# docker manifest --help
docker manifest is only supported on a Docker cli with experimental cli features enabled
root@localhost:~# export DOCKER_CLI_EXPERIMENTAL=enabled
root@localhost:~# docker manifest --help

Usage:    docker manifest COMMAND

Manage Docker image manifests and manifest lists

Commands:
  annotate    Add additional information to a local image manifest
  create      Create a local manifest list for annotating and pushing to a registry
  inspect     Display an image manifest, or manifest list
  push        Push a manifest list to a repository

Run 'docker manifest COMMAND --help' for more information on a command.
root@localhost:~#

@mkumatag
Copy link
Member

mkumatag commented Sep 5, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@calebamiles
Copy link
Contributor

/hold
until we decide how to move forward in a SIG Release meeting

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2018
@dims dims force-pushed the support-for-manifests-using-docker-experimental-cli-commands branch from b24e993 to e4eef22 Compare September 7, 2018 22:01
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2018
@dougm
Copy link
Member

dougm commented Sep 8, 2018

Built + pushed the cloud builder image with this PR, currently running a mock build:

% git rev-parse HEAD
e4eef2277db5b12485c94715ce8380ead96a3c78
% gcloud --project kubernetes-release-test builds submit --config k8s-container.yaml
latest: digest: sha256:4503c04b0eeab2185d63d45503e9bec3cf582244d1deba300dc7ad7505184a9d size: 3256
DONE
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

ID                                    CREATE_TIME                DURATION  SOURCE                                                                                             IMAGES                                                      STATUS
5e95c99c-6b28-47d2-9488-b05901df3b6e  2018-09-08T00:07:34+00:00  6M6S      gs://kubernetes-release-test_cloudbuild/source/1536365253.43-87389752b5ea4aaf8be67a86880669dd.tgz  gcr.io/kubernetes-release-test/k8s-cloud-builder (+1 more)  SUCCESS

@dougm
Copy link
Member

dougm commented Sep 8, 2018

Running a mock build with @dims ' k/release master branch via:

modified   gcb/stage.yaml
@@ -9,7 +9,7 @@ steps:
 - name: gcr.io/cloud-builders/git
   args:
   - "clone"
-  - "https://github.com/kubernetes/release"
+  - "https://github.com/dims/release"
 
 - name: gcr.io/$PROJECT_ID/k8s-cloud-builder
   dir: release

The new Docker checks are passing:

Step #1: ================================================================================
Step #1: CHECK PREREQUISITES  (1/6)
Step #1: ================================================================================
Step #1: 
Step #1: Checking required system packages: OK
Step #1: Checking required PIP packages: OK
Step #1: Skipping security_layer::auth_check...
Step #1: Checking Docker version: OK
Step #1: Verifying Docker CLI Experimental status: OK
Step #1: Checking write access to registry gcr.io/kubernetes-release-test: OK
Step #1: Checking write access to registry k8s.gcr.io: OK
Step #1: Checking cloud project state: OK
Step #1: Checking write access to bucket kubernetes-release-gcb: OK
Step #1: [2018-Sep-08 01:06:00 UTC] check_prerequisites in 9s

@dougm
Copy link
Member

dougm commented Sep 8, 2018

Note that the initial beta.1 build before reverting these changes had failed the check:
Step #1: Verifying Docker CLI Experimental status

@calebamiles calebamiles closed this Sep 8, 2018
@calebamiles calebamiles reopened this Sep 8, 2018
@calebamiles
Copy link
Contributor

/hold cancel
per discussion in the SIG Release mailing list

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2018
@calebamiles
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2018
@calebamiles
Copy link
Contributor

/assign @dims

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

lgtm - I can "revert" the build image to the current master branch or wait for this PR to merge and rebuild/push then. Just let me know what you prefer @calebamiles @dims

@dims
Copy link
Member Author

dims commented Sep 8, 2018

@dougm i don't have a preference either way. Since you are the only one who has the super powers :) i am ok with either call.

@dougm
Copy link
Member

dougm commented Sep 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calebamiles, dims, dougm, mkumatag

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants