Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Reworked managed plugin make to provide cleaner plugin names #1048

Merged
merged 3 commits into from
Mar 18, 2017
Merged

Reworked managed plugin make to provide cleaner plugin names #1048

merged 3 commits into from
Mar 18, 2017

Conversation

msterin
Copy link
Contributor

@msterin msterin commented Mar 18, 2017

See comments in the Makefile for explanations

tested by building the plugin

msterin-m01:plugin msterin$ make 
Using the following config:
DOCKER_HUB_USER msterin EXTRA_TAG -dev VERSION_TAG 0.13
PLUGIN_NAME msterin/docker-volume-vsphere:0.13-dev
=== Cleaning work files, images and plugin(s)...
rm -rf ./rootfs
rm -f docker-volume-vsphere
docker plugin rm msterin/docker-volume-vsphere:0.13-dev
Error response from daemon: plugin "msterin/docker-volume-vsphere:0.13-dev" not found
make: [clean] Error 1 (ignored)
docker rmi msterin/docker-volume-vsphere:rootfs
Error response from daemon: No such image: msterin/docker-volume-vsphere:rootfs
make: [clean] Error 1 (ignored)
== building Docker image, unpacking to ./rootfs and creating plugin...
cp ../build/docker-volume-vsphere .
docker build -q -t msterin/docker-volume-vsphere:rootfs .
sha256:b3c2cb36bb6b05669d59ef7a741844d1db0b6028d938db1ac494d1530369311e
mkdir -p ./rootfs
docker create --name tempContainer msterin/docker-volume-vsphere:rootfs
6ccebb443bb00c60799cefb7fd53374b9915b35c37404403f564216b0bf1d645
docker export  tempContainer  | tar -x -C ./rootfs
docker rm -vf  tempContainer
tempContainer
docker rmi msterin/docker-volume-vsphere:rootfs
Untagged: msterin/docker-volume-vsphere:rootfs
Deleted: sha256:b3c2cb36bb6b05669d59ef7a741844d1db0b6028d938db1ac494d1530369311e
Deleted: sha256:1baac8a3544ae7ef3f72dd996f56268ac5dee7910aa3f2fb27840d71ace05a00
Deleted: sha256:5ee6af2254cd9addc94b6d672453b0cedf1158b37f88c83388b030de496228e6
Deleted: sha256:be5f30465d3052086dc624b92eb77d63d52a0a146c20b9e9f8647803473bcf4f
Deleted: sha256:b9e62fae70d056a98d4e1058a7a4e1e3c67604f4a9086eee63f892f63e8d6238
Deleted: sha256:c79c9d0b0a6eaf31b3b2c234f3052c7e0fb16c18ee23cdad025c918ffbce935c
Deleted: sha256:ccded0ea386af6ab7994030e5d85e54fed4f6b116586f43d895c7b9926dedac9
-- Creating  plugin msterin/docker-volume-vsphere:0.13-dev ...
docker plugin create msterin/docker-volume-vsphere:0.13-dev .
msterin/docker-volume-vsphere:0.13-dev
Pushing msterin/docker-volume-vsphere:0.13-dev to dockerhub.io...
The push refers to a repository [docker.io/msterin/docker-volume-vsphere]
b6066c4d37e5: Pushed 
0.13-dev: digest: sha256:ffe0dd7640eeb1f274f0e6b6d0408a15f50d1c918c005edef0e43af5a31433a5 size: 519

Reworked managed plugin make to provide cleaner plugin names
4300254 
See comments in the Makefile for explanations
@msterin msterin self-assigned this Mar 18, 2017
@msterin msterin added this to the 0.13 milestone Mar 18, 2017
shuklanirdesh82
shuklanirdesh82 approved these changes Mar 18, 2017
View reviewed changes
Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

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

LGTM! some minor nits.

plugin/Makefile Outdated
# You can use the following environment vars to change the behavior :
#
# DOCKER_HUB_USER - name of user and dockerhub repo to use. Default is `whoami`
# Please 'docker login' there is you want to push
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you please improve inline comment? it sounds confusing to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what exactly is confusing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was not clear earlier with DOCKER_HUB_USER and Please 'docker login' there is you want to push .. after renaming it to docker_hub_repo it makes sense that DOCKER_HUB_USER needs to be set as environment explicitly if you are planning to push <docker_user_ids>/docker-volume-vsphere or cnastorage/docker-volume-vsphere as many of us are not having docker_user as whoami

No issues now as the variable got renamed now.

plugin/Makefile Outdated
EXTRA_TAG ?= -dev

# final tag
PLUGIN_TAG := $(VERSION_TAG)$(EXTRA_TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add/append os distro in the final tag?

If I understand correctly rootfs would be different across distros, please correct me here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it only depends on FROM <distro> in Dockerfile so it won't be different

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @msterin for the info!

pdhamdhere
pdhamdhere reviewed Mar 18, 2017
View reviewed changes
plugin/Makefile Outdated
#
# You can use the following environment vars to change the behavior :
#
# DOCKER_HUB_USER - name of user and dockerhub repo to use. Default is `whoami`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually a repository than HUB_USER

Copy link
Contributor Author

@msterin msterin Mar 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. DOCKER_HUB_REPO then. Fixed

plugin/Makefile Outdated
VERSION_TAG ?= $(shell echo $(GIT_TAG) | awk -F. '{printf ("%d.%d", $$1, $$2+1)}' )
EXTRA_TAG ?= -dev

# final tag
Copy link
Contributor

Choose a reason for hiding this comment

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

So when we cut a release, we need to set VERSION_TAG and EXTRA_TAG. Append to above comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to avoid enumerting all possibilities as I do not know how we will name the release. But an example is already there:

# DOCKER_HUB_REPO=vmware EXTRA_TAG= VERSION_TAG=latest make
#           --> resulting name vmware/docker-volume-vsphere:latest

pdhamdhere
pdhamdhere approved these changes Mar 18, 2017
View reviewed changes
Copy link
Contributor

@pdhamdhere pdhamdhere left a comment

Choose a reason for hiding this comment

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

Minor comments.

plugin/Makefile Outdated
# VERSION_TAG - How you want the version to look. Default is "current git tag +1 "
# EXTRA_TAG - additional info you want in tag. Default is "-dev"
#
# To check resuting settings use "make info"
Copy link
Contributor

Choose a reason for hiding this comment

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

typo=>resulting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

plugin/Makefile Outdated
# You can use the following environment vars to change the behavior :
#
# DOCKER_HUB_REPO - name of user and dockerhub repo to use. Default is `whoami`
# Please 'docker login' there is you want to push
Copy link
Contributor

Choose a reason for hiding this comment

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

"You need to log in, using the 'docker login' command to push."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rephrase

@msterin msterin force-pushed the minor-make.msterin branch from 3476c23 to e3543b2 Compare March 18, 2017 07:45
@msterin msterin merged commit 9cff7eb into master Mar 18, 2017
@msterin msterin deleted the minor-make.msterin branch March 18, 2017 07:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shuklanirdesh82 shuklanirdesh82 shuklanirdesh82 approved these changes

@pdhamdhere pdhamdhere pdhamdhere approved these changes

Assignees

@msterin msterin

Labels
cla-not-required
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
4 participants
@msterin @pdhamdhere @shuklanirdesh82 @vmwclabot