On edition of an intermediate PR in a chain, merging the PR would lead
to *it* being forward-ported, duplicating the PRs already created
from *its* source.
Add a check for PRs in the target branch with the same source,
suppresses the forward-porting of the newly merged PR.
Fixes#451 (hopefully)
append `git status` data to stderr, should be somewhat more
informative especially when a conflict is a DU (where the file has
been deleted on one side, so there is no conflict marker anywhere).
Fixes#461
- When updating the local repo cache, always capture both stdout and
stderr and log them out rather than having them in journalctl hard
to relate to the main log.
- In the git layer, capture stderr by default and log it automatically
on command failure.
The process did properly update the state, but not the squash state.
It's somewhat unclear whether the state should be fully reset and
require reapproval though. Maybe only the validation should be reset?
The CI will eventually run and either succeed (re-validating) or
fail (devalidating, hopefully) but I'm not entirely sure this is
correct.
Apparently I wrote this when I was a dumbshit and did not think that
Python sets are implemented completely separately from dicts
and *remain unordered*.
As a result, the simplistic set union would not properly conserve
record order, which it should.
Apparently the "reason phrase" has been removed from HTTP/2 and github
has enabled HTTP/2, meaning responses may or may not contain a reason
phrase depending whether they're HTTP/1.1 or HTTP/2. Now I don't
understand how this triggers because requests isn't supposed to
support HTTP/2, and in a shell I do get a reason, but w/e.
Anyway the reason is used by a few tests to check more specifically
for the response they're getting, as that's used as the assertion
message by `get_ref`.
I guess I could replace the AssertionError by an HTTPError
but... w/e. Seems simpler to just fallback to the generic reason based
on the status code. It fixes both failing
tests (test_straightforward_flow and test_delete_normal) with very
little change.
aka: make it clean
This build database_ids field was generated a few months, waiting for the database to update with
this new sceme before using it.
It is still a little early to use it in cleanup methods, but this can be used to
generate the connect links dynamicaly.
To follow al specs introduced in previous commit, main fa-sign-in button should link to the -all.
It will almost always be the first one in database_ids in alphabetic order.
Then, in the dropdown, all other database are listed.
This will fix the previously broken design_theme connect link (no base nor all).
For this purpose nginx regex needs to be adapted to accept database name with underscore.
Using database name as subdomain will set the web.base.url correctly when connection for the first time using this link
This will unfortunatelly break connect all and connect base links if dns and nginx are not setup.
This won't fix the web.base.url when connecting with the Connect (database selector) button either.
When a status cannot be sent to github, the status is created in
database but the `sent_date` field is not set.
Because of that, the status cannot be sent manually from the frontend.
With this commit, that kind of status can by tried again.
5cf3617eef intended to create merge
messages with only the content of PR descriptions before the first
thematic break. However this processing was incorrectly applied
to all messages being processed (meaning rebased / squashed commit
messages as well).
Properly apply thematic break processing to only commit messages
created from PR descriptions.
Before this, we would "roughly" select stagings by looking at stagings
whose heads matched a specific sha then validating them all. This
could perform extra validations on stagings once in a while but this
was assumed not to be much an issue, at least originally.
However two changes later on have contributed to this likely being the
cause of #429 (stagings never timing out):
* heads of the staging branches are uniquifier commits stored in the
heads map, but the actual heads of the stagings are also stored
there, some of which are no-ops (hence the uniquifiers) so assuming
repos A and B, if a staging contains PRs touching A then the head of
B actual will also be a head of B
* when a staging is validated, if it *contains* any pending result the
timeout limit gets bumped back
The issue here is that if a success / failure status is lost (which
would be the most common reason for timeouts) *and* someone has forked
and is regularly rebuilding a branch-head used as-is by a staging,
each of those rebuilds will trigger a validation of the staging, which
will find that one of the statuses is still pending (because we missed
the success / failure), which will bump up the timeout limit,
continuing until the branch stops getting rebuilt.
This is probably one of the reasons why some stagings last for *way*
more than 2h, though it is far from explaining all of them: 90% of the
stagings lasting more than *3*h end up succeeding. Tho it's always
possible that this is because someone notices and resends a success
for the missing status it seems somewhat doubtful. Oh well.
Also fix the incorrect log call on `update_timeout_limit` triggering.
* fix small error which generated an extra commit in case of conflict
probably (would create a `temp` commit, then put the conflict
information on an empty commit on top of it). Avoids committing the
cherrypick though a probable alternative would be to commit the
message with `amend`.
* improve the cherrypick header: instead of one log line per commit,
put all commits within the sole header log line (with a newline),
should make things less noisy
* put *all* log records below the correct logger (two of them were on
the toplevel logger)
* log a purported inflateInit error as warning, should be sent to
Sentry instead of having to wait for people to wonder why the thing
is completely broken
I'd forgotten that in order to better handle cases where the CI is
highly backed up (and / or slow for some reason), we actually update
the CI timeout to really take the last "pending" status as the "true
start" of the CI. This might explain why lots of stagings needed extra
time: as of right now, out of 28835 stagings
- 20086 had their timeout bumped by more than 15mn
- 6967 had their timeout bumped by more than 30mn
- 264 had their timeout bumped by more than 1h
- 30 had their timeout bumped by more than 2h
Add some logging every time the CI is bumped this way, so we have
better visibility into that event.
Closes#429
The mergebot has a feature to ping users when an approved PR or
forward-port suffers from a CI failure, as those PRs might be somewhat
unattended (so the author needs to be warned explicitly).
Because the runbot can send the same failure information multiple
times, the mergebot also has a *deduplication* feature, however this
deduplication feature was too weak to handle the case where the PR has
2+ failures e.g. ci and linting as it only stores the last-seen
failure, and there would be two different failures here.
Worse, because the validation step looks at all required statuses, in
that case it would send a failure ping message for each failed
status *on each inbound status*: first it'd notify about the ci
failure and store that, then it'd see the linting failure, check
against the previous (ci), consider it a new failure, notify, and
store that. Rinse and repeat every time runbot sends a ci *or* lint
failure, leading to a lot of dumb and useless spam.
Fix by storing the entire current failure state (a map of context:
status) instead of just the last-seen status data.
Note: includes a backwards-compatibility shim where we just convert a
stored status into a full `{context: status}` map. This uses the
"current context" because we don't have the original, but if it was a
different context it's not going to match anyway (the target_url
should be different) and if it was the same context then there's a
chance we skip sending a redundant notification.
Fixes#435
Before this change, a CI override would have to be replicated on most
/ all forward-ports of the base PR. This was intentional to see how it
would shake out, the answer being that it's rather annoying.
Also add a `statuses_full` computed field on PRs for the aggregate
status: the existing `statuses` field is just a copy of the commit
statuses which I didn't remember I kept free of the overrides so the
commit statuses could be displayed "as-is" in the backend (the
overrides are displayed separately). And while at it fix the PR
dashboard to use that new field: that was basically the intention but
then I went on to use the "wrong" field hence #433.
Mebbe the UI part should be displayed using a computed M2M (?)
as a table or as tags instead? This m2m could indicate whether the
status is an override or an "intrinsic" status.
Also removed some dead code:
* leftover from the removed tagging feature (removed the tag
manipulation but forgot some of the setup / computations)
* unused local variables
* an empty skipped test case
Fixes#439.
Fixes#433.
When a new branch is created between other branches, the process to
try and look for forward-port PRs to create to "fill in" currently has
no logging so it's very difficult to figure out why it decides not to
do something.
Add some logging to the process to try and better understand what
happens.
Fixes#441
Currently when creating *merges* the commit message is the
concatenation of the PR title and the PR body.
However it can be convenient to include description test which would
not be included in the commit message e.g. PR template
stuff. "Thematic breaks" seem like a good way to do this: the commit
message will only include the content preceding the first thematic
break, everything else is ignored (except headings which are not
ignored, double negations FTW).
Might be that that's a bit excessive and we should only ignore what
follows the *last* thematic break. Or that there needs to be a more
specific marker. We'll see.
Fixes#443.
Because the commands were collected in a `dict[Command, Params]` a
command could only be present once *in the mergebot* (the forwardbot
already collected commands in a list).
* Update mergebot commands parser to collect commands in a list.
* Improve override to allow a comma-separated list of CIs to override.
* Simplify the parsing of delegate to generate one delegate command
per target (slightly less efficient if multiple logins are provided
but that is likely extremely rare).
Fixes#445
On a cherrypick failing due to renamelimit issues, the cherrypick
would be retried after resetting the target to its *original* commit.
This only works correctly if the first commit works after a retry, if
a latter commit has to be retried then it gets re-cherry-picked onto
the target branch rather than its own parent.
Fix by remembering the previously successful cherry-picked commit and
resetting to *that*. However I can't really test it because it's
rather hard to get into a situation where the rename detection fails
using synthetic tests.
While at it, clean the logs by stripping the "performing inexact
rename detection" stuff from all stderr (both the CherrypickError from
which it was already stripped and the debug messages).
Fixes#444
When a build is reaching the run_run_odoo step, a database has to be
set. If none are found in the build params, the one from the last step
is choosen. Historically, the last one in the `Split` config was `all`
but now, the last one is `base`.
With this commit, if none are found in build params, `all` is choosen if
found in any install config steps. As a default, the one from the first
step is choosen.
Since 3657a65b20 docker_run is called outside of the step and python
steps have to set the `docker_params` variable. This breaks the computed
`log_list` because the string `docker_run(` is searched in python
steps code to determine if it's a docker_step.
With this commit, the `docker_params = ` is searched instead.
For now, the sticky flag is used to define bundle to use as target to upgrade.
The plan for future is to continue to test upgrade, even if the bundle is not sticky anymore.
This new flag will allow to remove sticky for old branche.
This will also allow to disable upgrda tests for still sticky branches when needed.
This commit also filter source bundle based on this flag
(before that all base bundle where used as source, even if not sticky)
Params mechanism are a way to avoid to rebuild with the exact same params.
Commit is the main identifier to know if two builds are the same, ususally
commit_link are not usefull to uniquify params, exept when the mergebase is
in use as in security check (based on diff).
We would also like to avoid to have red diff-based build in sticky/base branches when the
ci is ignored on a pr, but we want to keep the diff display on new batches
(lines and files changed).
We need a way to know if it is a is_base bundle. This was an initially forbidden behaviour
because of duplicate detection.
This commit add a batch_dependent flag. This will enable the use of the batch in the
fingerprint, making it unique per batch and disabling duplicate detection/ in this
case we can use the "is_base" information from the bundle + avoid false duplicate
detection if trigger is based on mergebase-commit diff. (pr based on another pr now merged)
Because github materialises every labels change in the
timeline (interspersed with comments), the increasing labels churn
contributes to PRs being difficult to read and review.
This change removes the update of labels on PRs, instead the mergebot
will automatically send a comment to created PRs serving as a
notification that the PR was noticed & providing a link to the
mergebot's dashboard for that PR where users should be able to see the
PR state in detail in case they wonder what's what.
Lots of tests had to be edited to:
- remove any check on the labels of the PR
- add checks on the PR dashboard (to ensure that they're at least on
the correct "view")
- add a helper to handle the comment now added to every PR by the 'bot
- since that helper is needed by both mergebot and forwardbot, the
utils modules were unified and moved out of the odoo modules
Probably relevant note: no test was added for the dashboard
ACL, though since I had to explicitly unset the group on the repo used
for tests for things to work it looks to me like it at least excludes
people just fine.
Fixes#419
Convert overridable CI to an m2m from partners, it's significantly
more convenient to manipulate as multiple users can (and likely will)
have access to the same overrides, add a name_search so the override
is easy to find from a partner, and provide a view for the
overrides (with partners as tags).
Also make the repository optional on CI overrides.
Fixes#420
When retrieving unknown PRs, the process would apply all comments,
thereby applying eventual r+ without taking in account their
relationship to a force push. This means it was possible for a
mergebot-unknown PR to be r+'d, updated, retargeted, and the mergetbot
would consider it good to go.
The possible damage would be somewhat limited but still, not great.
Sadly Github simply doesn't provide access to the entire event stream
of the PR, so there is no way to even know whether the PR was updated,
let alone when in relation to comments. Therefore just resync the PR
after having applied comments: we still want to apply the merge method
& al, we just want to reset back to un-approved.
An other minor fix (for something we never actually hit but could):
reviews are treated more or less as comments, but separate at github's
level. The job would apply all comments then all reviews, so the
relative order of comments and reviews would be wrong.
Combine and order comments and reviews so they are applied
in (hopefully) the correct order of their creation / submission.
Closes#416
2.29 changed the formatting of conflict labels (the stuff added at the
end of the closing conflict marker):
6cceea19eb
Used to be
>>>>>>> <commit>... <text>
now is
>>>>>>> <commit> (<text>)
Just ignore everything after the commit hash.
On the build error web page, a regular assigned error is not shown to
all users.
With this commit, a regular build error (not only random) will be shown
if the error is assigned.
When builds logs are parsed by using the contextual button, the client
stays on the same page even if a build error is created.
With this commit, the client is now redirected to the created/found
build error(s).
