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

Allow files to be used for bump and pre params #86

Closed
wants to merge 1 commit into from
Closed

Allow files to be used for bump and pre params #86

wants to merge 1 commit into from

Conversation

mattlqx
Copy link

@mattlqx mattlqx commented Jan 8, 2019

Allowing bump and pre params to be specified from files follows Concourse convention of passing data between resources by files. This will allow upstream tasks or resources to decide what the bump/pre level of a version should be.

There's a fair amount of copy/paste here. I'm new to golang so I'm not sure what best practices I'm missing about DRY here.

Fixes #75

@mattlqx
Copy link
Author

mattlqx commented Jan 10, 2019

I've unfortunately discovered that the in/get runs in total isolation so there doesn't seem to be a way to get files to it as inputs. Bummer. Not sure how to work around this. I have logic that I want to run prior to semver's get and then use its output as the bump files to the semver resource. I think to do it properly, all that logic would need added as an option to the semver resource in order to support this. I would've hoped that a task could handle all that arbitrary logic instead and could pass that into a resource, but I also get the distinction a bit better between the two now.

@vito
Copy link
Member

vito commented Jan 10, 2019

Yep, this can only work in put, not get. Do you actually need it in get or was it just for symmetry?

@mattlqx
Copy link
Author

mattlqx commented Jan 10, 2019

I'd like to have the resource plop the current version in for the pipeline to use without actually making writes to the backing source. This is useful to expose what the version will be in the UI before it actually happens, but also because I want to the artifacts to have been created with the appropriate and uploaded to their repository prior to actually writing the semver to the backing source. That's safest in my mind so that failed builds don't increment versions. But wanting the bump level to be determined dynamically makes my desire not fit into the get constraints.

I've started going down the road of specifying a "read only" param on the put/out that will do all the work except for writing to the backing source. I'm not sure how frowned upon that is since using a put in this way is probably breaking convention, but honestly it does exactly what I wanted get/in to do.

@vito
Copy link
Member

vito commented Jan 10, 2019

Sounds like you're trying to use the get kind of like a task. I always found it weird that the get supports bumping. Maybe this is another case for https://github.com/concourse/rfcs/issues/7 - and this resource could provide a general task for doing local version bumps?

@mattlqx
Copy link
Author

mattlqx commented Jan 14, 2019

I've updated this pull. My solution to fit within the rules of what Concourse currently allows with resources is to add a param that prevents semver from writing to the backend data source/driver during a put. If you wanted to deprecate bumping from get with this change, you probably could since put can effectively do everything that the get can do and more.

With this change, my pipeline looks something like this:

jobs:
- name: "Build Master"
  plan:
...
  - task: bump-level
    file: concourse/tasks/bump-level.yml
    params:
      COMMITS_FROM: master
  - put: version
    params: {bump_file: /tmp/build/put/bump-level/version_bump, pre_file: /tmp/build/put/bump-level/pre, driver_read_only: true}
< do work >
  - put: "Bump Version"
    resource: version
    params: {bump_file: /tmp/build/put/bump-level/version_bump, pre_file: /tmp/build/put/bump-level/pre}
...

Haven't quite figured out why I can't use relative paths for the bump_file/pre_file.

EDIT: looks to be because the process runs without a current working directory curiously. I see the first argument is used as a join on the path.

@vito
Copy link
Member

vito commented Jan 15, 2019

Sorry but I don't think I can get behind that approach either. :/ This feels like it's going against what the resource interface is designed for and it really should just be a task, since you're intending to perform an action and get an output (the locally bumped version) without any side-effects. Performing a read-only put is a bit of an oxymoron and I'm a little worried about the implications that would have on the rest of the pipeline, as the version would still be recorded in the database despite never actually being published.

As of right now I would feel more comfortable with expanding discussion around concourse/rfcs#7 rather than merging this in. This is one of our core resource types so it's important that we be somewhat strict about the patterns they encourage. Sorry again!

@vito vito closed this Jan 15, 2019
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.

2 participants