chore(ci): add verify-commits make target #138
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
scripts/verify_commits
Outdated
| function fetch_remotes() { | ||
| local -a remotes | ||
| remotes=( | ||
| 'api' | ||
| 'operator-registry' | ||
| 'operator-lifecycle-manager' | ||
| ) | ||
| for r in "${remotes[@]}"; do | ||
| local url | ||
| url="[email protected]/operator-framework/${r}.git" | ||
| if git remote show "${r}"; then | ||
| git remote set-url "${r}" "${url}" | ||
| else | ||
| echo "remote ${r} doesn't exist" | ||
| git remote add "${r}" "${url}" | ||
| fi | ||
|
|
||
| git fetch "${r}" master | ||
| done | ||
| } |
There was a problem hiding this comment.
there's a preexisting function for this hidden away, but it depends on the tracking file, which I don't think we need going forward. should I drop that and clean up the unused scripts in this PR too?
There was a problem hiding this comment.
I think that commit could make a nice PR of its own. I typically tend towards smaller, more tightly scoped PRs. WDYT?
There was a problem hiding this comment.
Yeah that's fine with me - we can keep those changes outside of this PR.
|
/test verify |
…shift#138) Upstream-repository: api Upstream-commit: a3fa2bd8d7c7f540824cda1b06f6677f198cfbb8
njhale
force-pushed
the
verify-commits
branch
13 times, most recently
from
4818336 to
0e040da
Compare
njhale
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
njhale
changed the title
scripts/verify_commits.sh
Outdated
| function fetch_remotes() { | ||
| local -a remotes | ||
| remotes=( | ||
| 'api' | ||
| 'operator-registry' | ||
| 'operator-lifecycle-manager' | ||
| ) | ||
| for r in "${remotes[@]}"; do | ||
| local url | ||
| url="https://github.com/operator-framework/${r}.git" | ||
|
|
||
| git remote add "${r}" "${url}" || git remote set-url "${r}" "${url}" | ||
| git fetch "${r}" master | ||
| done | ||
| } |
There was a problem hiding this comment.
There's probably value in extracting this to a common/helper library such that people can source this in the future, but that can be done later down the line.
scripts/verify_commits.sh
Outdated
| mapfile -t sr < <(upstream_ref "${commit}" || exit 1) | ||
| if (( ${#sr[@]} < 2 )); then # the ref contains a tuple if the values were properly parsed from the commit message | ||
| # couldn't find upstream cherry-pick reference in the commit message | ||
| # assume it's downstream-only commit | ||
| info "${commit:0:7} does not reference upstream commit, verifying as downstream-only" | ||
| else | ||
| # found upstream cherry-pick reference in commit message | ||
| # verify as an upstream sync | ||
| info "${commit:0:7} references upstream commit (${sr[*]}), verifying as upstream sync" | ||
| verify_upstream_sync "${sr[0]}" "${commit}" || exit 1 | ||
| fi |
There was a problem hiding this comment.
Hmm it's possible I'm misreading this but this seems like it's avoiding the edge case where we're pulling in an upstream commit, but someone forgot to populate that upstream commit metadata into the pulled down commit?
There was a problem hiding this comment.
Ah, yeah I glossed over the commented text - it seems like we'd want to enforce this behavior as a stop gap until automation has been introduced. Would it be more reasonable to assume that anything that touches the staging tree needs to be treated as an upstream commit? I still need to dive into that upstream_ref function so I may be missing some context here.
njhale
force-pushed
the
verify-commits
branch
2 times, most recently
from
69bbeb0 to
9a6a711
Compare
scripts/verify_commits.sh
Outdated
| if (( ${#upstream_repos[@]} != 1 )); then | ||
| err "downstream staging commit ${downstream_commit} references an invalid number of repos:" | ||
| err "${upstream_repos[@]}" | ||
| err "staging commits must reference reference a single upstream repo" |
There was a problem hiding this comment.
nit: staging commits must reference a single upstream repo
| return 1 | ||
| fi | ||
|
|
||
| if git branch -r --contains "${upstream_commits[0]}" | grep -vq "${upstream_repos[0]}/.*"; then |
There was a problem hiding this comment.
does this cause problems if we need to revert commits upstream? I suppose in that case we need to explicitly revert the commit in order, but it could be a problem if we're on a release branch where that is not trivially possible
There was a problem hiding this comment.
does this cause problems if we need to revert commits upstream?
reverting a commit is additive -- it creates a new commit that undoes the changes of the target -- so in theory, the original commit should still exist (if I didn't misunderstand the concern).
There was a problem hiding this comment.
yeah, we just need to enforce that we're not tossing history and it's not a problem. I think this is fine I just wanted to call it out as a potential problem if upstream ever got borked.
| local outside_staging | ||
| outside_staging="$(git show --name-only "${downstream_commit}" -- ":!${staging_dir}" ':!vendor :!manifests')" | ||
| if [[ -n "${outside_staging}" ]]; then | ||
| err "downstream staging commit ${downstream_commit} changes files outside of ${staging_dir}, vendor, and manifests directories" |
There was a problem hiding this comment.
One quick thought: would we necessary group an upstream-specific commit and the requisite manifest changes in the same commit? Grouping the staging tree changes + any vendor changes makes sense to me, especially in the reversion case, but still trying to wrap my head around whether we'd like to enforce that manifest changes need to also live in that same commit. Any ideas?
There was a problem hiding this comment.
but still trying to wrap my head around whether we'd like to enforce that manifest changes need to also live in that same commit. Any ideas?
The current code ensures that no staging commit touches a staging directory and a directory other than ./vendor or ./manifests. It doesn't enforce that changes to manifests occur in a staging commit, if that's the concern.
There was a problem hiding this comment.
Ah, my mistake likely glossed over too quickly - this implementation is fine with me.
Add a Make target that verifies commits added between the current head and master: - reference only one upstream, if any - if referencing an upstream, only make changes to the vendor, manifests, and/or respective staging directory - if not referencing an upstream, don't make changes to the staging directory
|
/lgtm |
|
/lgtm |
|
This is a script change that doesn't affect any of the running e2e tests in any way. Overriding ci /override ci/prow/e2e-aws-olm |
|
@kevinrizza: Overrode contexts on behalf of kevinrizza: ci/prow/e2e-aws-olm, ci/prow/e2e-gcp, ci/prow/e2e-upgrade In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Add the
verify-commitsMake target to verify that commits added between the currenthead and master:
manifests, and/or respective staging directory
Also, include
verify-commitsunder theverifytarget. This ensures the verification is executed by Prow w/o requiring any updates to the Prow config.