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 more complex programmatic tags #64

Closed
wants to merge 1 commit into from

Conversation

jgreat
Copy link

@jgreat jgreat commented Jun 24, 2016

We would like to use a tagging scheme based on the version of our nodejs applications defined in in the project package.json file. There doesn't seem to be way to inject this into Drone at build time. So as a build task, we are create a simple yaml file with a list of tags (.droneTags.yml) in the root of the project and override the passed in Tags with the contents.

We created this nodejs utility to geneate tags for us https://www.npmjs.com/package/buildgoggles

If you want to name the yaml file something else I'm more then happy to change.

.droneTags.yml

tags:
  - latest
  - v1.0.2-1
  - owner_project_feature-some-long-name_1.0.2_1_ae82ac97

@donny-dont
Copy link
Contributor

@bradrydzewski any thoughts? One thing I would think would be nicer is if it were something like template where the value is either in the string or from a file on disk. I don't like the hard coded nature of it.

@jgreat
Copy link
Author

jgreat commented Jun 24, 2016

How about a tag_file: option for the plugin?

@bradrydzewski
Copy link
Member

bradrydzewski commented Jun 24, 2016

@jgreat in this case I recommend keeping your own fork of the plugin. With 0.5 we are moving most plugins out of the official drone-plugins org and keeping a small set of simple, core plugins. We encourage extending and forking plugins, and then if those extensions end up being useful to the broader community we can bring them into the core.

You could probably event extend without writing any Go code. You could use this Dockerfile:

FROM plugins/docker
ADD docker.sh /bin/
ENTRYPOINT [ "/bin/docker.sh" ]

And wrap with a simple shell script (docker.sh):

#!/bin/sh
export PLUGIN_TAGS=$(cat VERSION)
/usr/local/bin/dockerd-entrypoint.sh /bin/drone-docker

In fact this the approach we're going to be taking with the gcr plugin.

@bradrydzewski any thoughts? One thing I would think would be nicer is if it were something like template where the value is either in the string or from a file on disk. I don't like the hard coded nature of it.

While I agree this would be useful it is just not technically possible. The yaml is parsed and processed before the repository is cloned. This is core to how drone behaves and is just not easily changed. Sorry to disappoint :(

@donny-dont
Copy link
Contributor

Yea in hindsight I think having a .drone.js would solve this problem better.

@jmccann
Copy link

jmccann commented Oct 12, 2016

@bradrydzewski I was wondering if this could be reconsidered. I've had people express interest in this functionality as well.

I'm not sure how:

... it is just not technically possible. The yaml is parsed and processed before the repository is cloned.

Doesn't seem like it'd be very hard to implement something to make it work. Weather it's changing the Dockerfile as you expressed above to allow reading of values from file or even having the plugin itself read values from a file (that could be a parameter itself) directly (which this PR seems to provide?)?

I'm just hesitant to fork this plugin for a couple reasons:

  1. Harder to maintain going forward to pull in any updates from core back into our fork
  2. Others in community seem to have expressed interest in this functionality
  3. I don't want to have to override DRONE_PLUGIN_PRIVILEGED on my agents to allow running a forked plugin in privileged mode.

@bradrydzewski
Copy link
Member

I think the answer is in this block of code:
https://github.com/drone-plugins/drone-docker/blob/master/main.go#L144:L146

We have the ability to load environment variables from a file. The file name could be exposed in the yaml allowing you to dynamically configure the plugin from a file. This same process could be applied to all plugins, for all variable types.

@jmccann
Copy link

jmccann commented Oct 21, 2016

Thanks for the comment @bradrydzewski

So do I just need to add add an EnvVar: "PLUGIN_ENV_FILE", to https://github.com/drone-plugins/drone-docker/blob/master/main.go#L132-L135 ? Then I'd be able to specify something like the following simple example, correct?:

pipeline:
  build:
    image: ruby:2.3
    commands:
      - echo 'PLUGIN_TAGS=0.1.0' > my_env_file

  docker:
    image: plugins/docker:latest
    username: jmccann
    repo: jmccann/my-app
    file: Dockerfile
    insecure: false
    env_file: my_env_file

If that sounds about right I don't mind doing the work and creating a PR if you think you'd merge it. 😉

@bradrydzewski
Copy link
Member

@jmccann yes something that like that should work. The only thing to note is that there may be an order of operations issue that needs to be taken into account.

I'm wondering if this line of code would even work, since the flags will be parsed and set already. not sure if loading the environment variables would have any impact ...

func run(c *cli.Context) error {
    if c.String("env-file") != "" {
        _ = godotenv.Load(c.String("env-file"))
    }

@bradrydzewski
Copy link
Member

just to confirm, the below code doesn't actually do anything. So we'll need a way to load (or re-load) the environment variable and flags based on alternate environment input.

    if c.String("env-file") != "" {
        _ = godotenv.Load(c.String("env-file"))
    }

there is actually a package that facilitates this sort of thing that could possibly be addapted to the godotenv file. See https://godoc.org/github.com/urfave/cli/altsrc

@jmccann
Copy link

jmccann commented Oct 27, 2016

@bradrydzewski I took a look at altsrc and had a hard time with it. Probably mostly to due with my lack of golang fu.

I was able to get "env-file" to work though. Was wondering if you'd mind taking a look @ jmccann@830ed2a

I'm hoping that would be acceptable as MVP (minimal viable product) since currently env-file does nothing anyways. Then either you or someone else could revisit doing it with altsrc if there is added value to doing it that way (or some other way) instead.

If you think it looks good I'll PR it.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants