sclorg / ci-scripts Goto Github PK

Sanity checker sends an email about .devel-repo and .exclude files.
It would be nice if this information is sent only in case if container image reached GA or EOL.

Sender shown in email:
From: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "", [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "."@scl-nightly-builds.localdomain, [email protected], [email protected], [email protected]

Reviewing PR sclorg/s2i-ruby-container#316, we've hit a corner-case which uncovered this issue.

Problem

How to reproduce:

• PR is created, on top of master branch.
• In the meantime the master branch gets updated with non-conflicting changes (e.g. updating common scripts).
• While reviewing the PR, issues are found, fix gets pushed and CI re-runs.
• The CI gets the state of the current PR (with the master commits from that time), although there are additional non-conflicting changes.
• Lack of the changes (some other fix) can cause the CI to fail, although it would pass when rebased.
• The actual PR will get auto-rebased on merge.

In current state:

• The manual rebase serves no purpose, apart from actually running the CI with correct state.
• In most cases, the PR will not get rebased with latest changes, and the CI result is not the latest one (e.g. missing some tests added in previous PR).
• Therefore the results from the CI do not reflect the "final state", IOW state that will occur when the PR gets merged, as it was auto-rebased.
• The very next PR (with no changes) can actually fail, as some breaking changes could be merged when an outdated test suite (or common scripts) was used for testing the previous PR.

(Please to correct me if I'm mistaken.)

Solution

I'd simply "auto-rebase" for the PR testing (rebase on latest origin/master), not just for the actual merge. This is the cleanest and most accurate solution. I did actually expect that behaviour, and I think it's a correct one.

In case when auto-rebase is not possible, the CI should not run either way.
I don't think the current state has any benefits. Are there some corner cases that I've missed?

Previously resiliency was listed as a reason to keep current state. This change would not do any difference in that state, as only the commits already in this repo are used. IOW container-common-scripts does not get auto-updated for CI testing, and any faulty change commited there will not be propagated, unless actually merged in this repo.

Bottom line, updating the 'common' commit (or any other change) is not a part of the PR, and always rebasing to the latest commit is not a requirement for merge as of now (because of the auto-rebase), unless the change is a conflicting one. F.e. gitlab has a rebase button on the PR, as it doesn't do auto-rebase the PR.

ci-scripts are cloned twice.

The first time, which is correct ci-scripts are cloned here: https://github.com/sclorg/rhscl-container-ci/blob/master/yaml/builders/clone-ci-scripts.yaml

But this script (https://github.com/sclorg/container-common-scripts/blob/master/update-generated.sh) deletes it.
The workaround is here: https://github.com/sclorg/ci-scripts/blob/master/jenkins_ci/commit-into-generated-branch.sh#L17