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 paths being passed to source configuration #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahume
Copy link
Owner

@ahume ahume commented Nov 23, 2016

Allow PR jobs to only be triggered when relevant
parts of the repository have been changed.

This is an investigation into how easy or otherwise this will be to support. I have some further things to investigate...

This should also support ignore_paths config, to match 1:1 with git-resource. Also is globbing behaviour correct? Does it support a list of different globs?
I've only added paths support to check. What (if anything) needs to be done for in or out?
NB: I haven't written a concourse resource before, nor do I write Ruby very much.

Allow PR jobs to only be triggered when relevant
parts of the repository have been changed.
@Seraf
Copy link

Seraf commented Dec 1, 2016

Looks great and it's a small change :)

Are you using it on your side ? Do you have a docker image with these changes to test it ?

@ahume
Copy link
Owner Author

ahume commented Dec 1, 2016

https://hub.docker.com/r/andyhume/pullrequest-resource/

It seems to work within the constraints set out above. ie. a single path value. However, there's likely an issue with statuses in the PRs. When there is one resource version per PR, it is simple to set a status on the PR and use that to determine old/new versions. When you can have multiple versions (one per path) of a PR, then the statuses need to be managed independently, one per path. And this has not been implemented yet.

I think this could lead to race-conditions where one path sets a status of "pending" and then the /check of another path doesn't find a new version for that PR because it already has a concourse-ci pending status attached to it. But I haven't had time to work further on this.

@Seraf
Copy link

Seraf commented Dec 1, 2016

Awesome.

About this problem, I may be wrong, but the ref version is the commit sha, am I right ?
There's one modification per commit on paths, why would you like to have a version per path ?
Giving multiple path as parameter should be a or condition for the check :

If there's pathA or pathB in the files commited, then it creates a new pr version with the commit id. Else, it doesn't create a new version but return the old one.

Can you explain deeper your use case please ?

@jtarchie
Copy link

jtarchie commented Dec 7, 2016

Do you want to support paths and ignore_paths like the git resource? They work inclusively.

@ahume
Copy link
Owner Author

ahume commented Dec 7, 2016

@jtarchie Yup, I'll try and support the same paths/ignore paths matching.

@Seraf The issue (I think) is with sending status updates back to Github. With paths support, there could be multiple parallel jobs triggered from one PR. The status of these builds need to be sent back to Github individually, because they are used by the resource to determine if a version/path should be built...

  def ready?
    statuses.empty?
  end

(in /assets/lib/pull_request.rb).

Without this, when checking for new versions the resource won't know what paths have already had jobs started. If one has started, then statuses.empty? won't ever be true, and no further jobs will be triggered. If all jobs start at the same time, this probably won't matter, but there's the race condition. So I think something needs to be implemented to solve this but i'm not exactly sure what yet.

@Seraf
Copy link

Seraf commented Dec 7, 2016

@ahume I may be wrong, but why not using the context parameter for that to have a different status per path/pr-resource ?

For example, I have in my single repo a directory A and a directory B.

I create a pr-resource that listen on directory A, and will output the status in the context directory-A, then, even if the B fail on my pull request, it's gonna be a different status for the context directory-B

@Seraf
Copy link

Seraf commented Dec 7, 2016

Also, to add some stuff, I tried to use a path array :

- name: source-website-v2-prod
  type: pull-request
  source:
    uri: [email protected]:xxxx/xxxxxxxx.git
    repo: xxxxx/xxxxxxxxxxxxxxx
    access_token: {{github-access-token}}
    private_key: {{github-private-key}}
    paths:
      - website-v2/prod
      - website-v2/module-website-v2

It gives me this error on the check:

resource script '/opt/resource/check []' failed: exit status 1

stderr:
/opt/resource/lib/repository.rb:38:in `fnmatch': no implicit conversion of Array into String (TypeError)
	from /opt/resource/lib/repository.rb:38:in `block in pr_matches_paths?'
	from /opt/resource/lib/repository.rb:37:in `map'
	from /opt/resource/lib/repository.rb:37:in `pr_matches_paths?'
	from /opt/resource/lib/commands/check.rb:35:in `next_pull_request'
	from /opt/resource/lib/commands/check.rb:15:in `output'
	from /opt/resource/lib/commands/check.rb:52:in `<main>'

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.

3 participants