Conversation
- added base_context - added syntax highlighting to README - removed obsolete groundcrew section - added `put` context param to override the source param - created makefile for ease of development / testing - bumped `jq` version in makefile
christophermancini
force-pushed
the
develop
branch
from
758cc3b to
679927c
Compare
dpb587
left a comment
dpb587
left a comment
There was a problem hiding this comment.
Thanks for the pull request, and sorry for my delay. Generally changes seem good, but added a concern about the base_context change. Happy to chat more.
| * **`repository`** - the owner and repository name, slash delimited (e.g. `dpb587/github-status-resource`) | ||
| * **`access_token`** - GitHub API access token from a user with write access to the repository (minimum token scope of `repo:status`) | ||
| * `branch` - the branch currently being monitored (default: `master`) | ||
| * `base_context` - prefix for the context label (default: `concourse-ci`) |
There was a problem hiding this comment.
Can you describe the use case for this a bit more?
Assuming it makes sense, I don't think it should be defaulted (since it'd be a large, breaking change). So I think this should be adjusted to actually be the full prefix (where the user sets it to concourse-ci/ if they want this behavior) and then remove the slashes deeper in code.
There was a problem hiding this comment.
There are a few reasons for this change:
- it will make this resource consistent with the github-pr-resources, providing a more unified user experience across Concourse
- we have a staging cluster for testing new versions of Concourse, this allows us to have pipelines on this staging cluster use a prefix of
concourse-staging-ci - if you are testing changes to a pipeline before applying them to the primary pipeline, you can use this to differentiate between the two, e.g.
concourse-ci/dev
There was a problem hiding this comment.
W.r.t. breaking change, we could just generate a new 2.2.0 tag for the current version, then merge and generate a 3.0.0 tag. By not having the concourse-ci be the default, then it would deviate from the github-pr-resource. 🤔
| --arg target_url "$( echo "$target_url" | buildtpl )" \ | ||
| --arg description "$( cat $description_path )" \ | ||
| --arg context "$source_context" \ | ||
| --arg context "$(echo "$source_base_context/$cntxt" | buildtpl)" \ |
There was a problem hiding this comment.
Correct. It should not break status lookups via check as long as the user does not include any env vars in the resource source context.
|
@dpb587 just an FYI, we have been running the version of this resource on one of our most important pipelines internally since I created this PR without issue. Likely it has seen somewhere around 2000 builds or more since then. |
Temporarily bump the limit to 100 to avoid following cursors
Similar paging functionality
Add multi-page status results support
4 participants
putcontext param to override the source paramjqversion in makefile