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

Support paths configuration parameter #42

Closed
ahume opened this issue Nov 17, 2016 · 21 comments
Closed

Support paths configuration parameter #42

ahume opened this issue Nov 17, 2016 · 21 comments

Comments

@ahume
Copy link

ahume commented Nov 17, 2016

git-resource has paths and ignore-paths parameters which we heavily rely on in our monorepo style projects to test/build/publish only the apps that have had src code changes. Is this something that could also be supported by pullrequest-resource?

@jtarchie
Copy link
Owner

The resource relies heavily on the Github API for getting new PRs. It does not provide, to my knowledge, a way to specify paths.

Otherwise, the resource would have to check out each PR, then run the git logs, etc. It is just a lot of work for the check step to do.

I don't think I could support this efficiently.

@jtarchie
Copy link
Owner

It looks like there is an API endpoint to get a list of files.

@ahume
Copy link
Author

ahume commented Nov 18, 2016

Thanks @jtarchie

It looks like there is an API endpoint to get a list of files.

Meaning it could be supported efficiently? Or that we should implement the check as a task in the pipeline?

@jtarchie
Copy link
Owner

implement the check as a task in the pipeline sounds like the most efficient way, right now. This is going to be hard. :-/

@Seraf
Copy link

Seraf commented Nov 19, 2016

Hello, have you an example how can the check be handled in the pipeline ?

We have the same case : multiple subdirectories on the same repository, and wanted to have a pipeline per subdirectories.

Thanks !

@jtarchie
Copy link
Owner

Have a task or a job that does the equivalent git log -- paths.

@jtarchie
Copy link
Owner

The pull request resource is meant to be the pr resource. It provides enough meta data for the user to implement their own workflow.

I've had many requests to supports different workflows. They are never consistent. I'd like to keep the resource minimal, while only supporting options of the GitHub API.

Supporting all the options of the git resource is difficult as it only pertains to a single git repo, branch, tag, etc. Trying to implement the same filtering introduces a cross section of API and git commands.

Your best bet to have the exact functionality of the git resource it to use https://github.com/robdimsdale/concourse-pipeline-resource to create a pipeline on the fly.

If someone can propose an efficient way to implement git resource features here, I'm open. There may be a better way to do this. Otherwise I'd like to close this issue.

Thoughts?

@Seraf
Copy link

Seraf commented Nov 19, 2016

I think there's a real need on this feature request, and as it has been implemented on the official git resource, it seems legit to figure into this resource as well. I don't get the added value to complexify pipelines to handle something that's really tied to the pr-resources : use the PR api of github to know which files has been modified and know if the resource need to be triggered or not.

I didn't get into the code yet, but according to the git-resource, they just get a list of pattern as parameter and pass it to git (yes, here git binary do all the work).
What I suggest here, is to do this api call : https://developer.github.com/v3/pulls/#list-pull-requests-files and compare the patterns provided as parameters with the list of changed files. If one of these match, then the check should trigger, else, it should not see a new version of the PR.

I'm also open to debate on it, but I have some difficulties to imagine how much it complexify a pipeline just to know if the PR should trigger or not the pipeline if we have to do it manually.

I'm not very skilled with Ruby, but I can help to test or anything to help on this issue :-)

@jtarchie
Copy link
Owner

Please see my earlier posts.

You have a real need because this is your workflow. I've not actually encountered a consistent workflow for PRs.

Users have reported the PR resource can be slow because of the number of API calls. Hitting that endpoint could be troublesome. The alternative of doing s git checkout and performing the operation could be heavy as PRs can be expensive to checkout.

Im still in inclined to limit the functionality of the resource, but provide enough meta information and the gotten checked out git repo for others to implement their workflow.

@Seraf
Copy link

Seraf commented Nov 20, 2016

@jtarchie I've seen your earlier posts, but I'm still not convinced. Here, we will need to do an api call to github to know the list of changed files. This is the goal of this resource to interact with github, I don't get why it should live in another resource / another task.

About performance, if there's no ignore_paths or path there won't be any api request. A warning in the readme that it will impact the performance will be enough and everyone will be happy to use this feature if needed.

My workflow is the same that lot of people : I'm using a single repository that contains little projects in subdirectories to avoid creating 100 dedicated repositories per customer we have in my company. And obviously, @ahume had the same need using a monorepo style.

@ahume
Copy link
Author

ahume commented Nov 20, 2016

I can see both points of view. A single extra API call seems negligible, but if it's not supporting the core use-case - it's somebody's judgement call. :)

My next problem is figuring out how to implement this in the pipeline. There's so much flexibility with resources/tasks and how they interact that it's not at all clear how to create implementations for very common CI/CD scenarios. My organisation has serious CI fatigue, and concourse would seem to solve many of the issues we have, except for the difficulty for engineers to recreate pretty much all but the simplest of workflows.

Anyway, that's irrelevant to the issue, so I'll leave you to decide on its future. :)

@ahume
Copy link
Author

ahume commented Nov 20, 2016

For example, I've been pointed to the concourse-pipeline-resource twice now. So the suggestion (if I understand it) is to read the name of the branch from the PR, and then dynamically create a new pipeline file that uses git-resource to sync the right branch, run the build (including updating the PR metadata in github (assuming pullrequest-resource wouldn't be able to do this any more) and clean-up after itself. The whole design of that seems incredibly round-about compared to just being able to trigger from particular path inputs.

I feel like we're back to the poor way we've been doing it in Travis, whereby the job itself checks if its paths have changed and just bails early with success if they have not. I'd really hoped to do better than that, but I don't feel that the above is better.

@jtarchie
Copy link
Owner

jtarchie commented Nov 20, 2016

It is hard to support all the different use cases. They are just not consistent.

These are just some of the requests that I've had:

  • people that use specially named branches
  • be able to trigger a PR based on comment
  • return all the tags specific to the PR
  • provide all the comment meta-data
  • filter PR branches by names

Each one for a unique use case.

I cannot support each individual case in which someone uses pull requests. They would each affect each other in different ways because of the composing of filtering options that each trigger. Since the GraphQL is not there yet, I'm not so inclined to build it myself.

The resource can provide as much meta information that can be provided for you all to perform these actions.

@Seraf
Copy link

Seraf commented Nov 20, 2016

I'm still a beginner with concourse but the pipelines already looks complex to do basic things for me. I understand your point of view, but that's also what parameters are for in my point of view.
Then, if you're not going to implement this, I think we can close this issue.

Can you please give an example about how to do extra stuff using your resource ? I think it will stop these custom workflow issues by providing an example of how to do that on the README.
If the solution is to generate a pipeline on the fly, then I think I will fork this resource because it's not really KISS for me.

Thanks for your time :)

@Seraf
Copy link

Seraf commented Nov 20, 2016

Also I've a question on this workflow : if I have two pipelines running both the PR resource on the same repository, what will be the behavior if the path check isn't part of the PR resource ? Does that means one will trigger but not the other pipeline ? Even if the pipeline should be namespaced last time I had the feeling that there was kind of sharing version between both pipelines. That one checked the new PR (but was not the right pipeline, that's why I'm asking the path feature) but the right one didn't trigger, stuck on checking for a new version

@ahume
Copy link
Author

ahume commented Nov 23, 2016

@Seraf If you want to collaborate on a fork of this, I've done a small bit of work to support paths as a source config.

ahume#1

It only supports check, and I need some input on what else needs to be done, as I'm not that familiar with the concourse architecture and how resources map to get/puts of jobs, etc... I'm also not a Rubyist, so code style comments greatly appreciated.

@ahume
Copy link
Author

ahume commented Nov 23, 2016

@jtarchie In reference to the above, would be great to get your input as well. I have a suspicion I may be under thinking this.

@Seraf
Copy link

Seraf commented Nov 23, 2016

Awesome @ahume ! I don't think in and out would benefit from having a path parameter. The responsible to check for new version is the check executable.

As you did, if there's a path parameter, then it check if the pr is a matching candidate or not. About ruby I don't think I will be very helpfull as I'm not really skilled with this one.

jtarchie pushed a commit that referenced this issue Dec 9, 2016
@jtarchie
Copy link
Owner

jtarchie commented Dec 9, 2016

@ahume and @Seraf, please see this commit.

It should be on jtarchie/pr:test docker image for testing. If you works for you I'll release it!

@Seraf
Copy link

Seraf commented Dec 12, 2016

Thanks @jtarchie, it's awesome :)

I will give a try this week to confirm it's working. Thanks !

@jtarchie
Copy link
Owner

Closing on release of v20. Please open up new issues if there are bugs or future feature requests.

nuclearnic pushed a commit to nuclearnic/github-pullrequest-resource that referenced this issue Sep 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants