Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#4354 closed defect (fixed)

Release automation commits and pushes local changes

Reported by: fhd Assignee: tlucas
Priority: P3 Milestone:
Module: Automation Keywords:
Cc: sebastian, trev, kvas, tlucas Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29508667/

Description (last modified by tlucas)

Environment

This happened during the Adblock Plus for Chrome/Opera/Safari 1.12.2 release, base revision was 44641c853190.

How to reproduce

  1. Clone the adblockpluschrome and downloads repositories. Important: Make sure to configure different remotes than the central upstream repositories (i.e. local copies of the repositories), the release automation automatically pushes changes.
  2. Make an arbitrary local change to the adblockpluschrome working directory, do not commit this.
  3. Still in the adblockpluschrome working directory, run build.py -t chrome release -k adblockpluschrome.pem -k adblockplussafari.pem 1.12.2 to perform the 1.12.2 release. (The version number you use doesn't matter.)

Observed behaviour

The local changes you made in the adblockpluschrome repository will be committed along with the release commit (the message starts with Releasing Adblock Plus).

Expected behaviour

The release automation shouldn't even start when the repository is dirty. Instead it happily commits the changes and pushes them. That could lead to nasty issues.

Any untracked or uncommitted local changes should cause the release automation to print an error-message and automatically abort the process.

The user should be prompted with detected outgoing changesets, along with a question whether to continue or not:

If you proceed with the release, they will be included in the release and pushed.
Are you sure about continuing the release-process?
Please choose (yes / no):

What to test

  • automatic abortion on
    • any uncommitted local change
    • any untracked local change
  • list of outgoing changesets (local-only changesets)
  • Abortion or or continuing of the release-automation, according to the users choice

Change History (22)

comment:1 Changed 3 years ago by fhd

  • Summary changed from Release automation commits local changes if there are any to Chrome/Opera/Safari release automation commits and pushes local changes

comment:2 Changed 3 years ago by sebastian

  • Cc trev kvas added
  • Priority changed from Unknown to P3
  • Ready set

comment:3 Changed 3 years ago by trev

  • Summary changed from Chrome/Opera/Safari release automation commits and pushes local changes to Release automation commits and pushes local changes

Changing title back - our release automation generally behaves that way, for Firefox as well.

Note that the proposed change won't solve the scenario where you already have local commits - these will be pushed as well. I guess that release automation could check for that as well.

comment:4 Changed 2 years ago by tlucas

  • Cc tlucas added

comment:5 Changed 2 years ago by tlucas

  • Owner set to tlucas

comment:6 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:7 follow-up: Changed 2 years ago by trev

I'd suggest a hard abort for uncommitted changes. While in the other scenarios I can imagine answering "yes, continue with the release" occasionally, uncommitted changes which will end up being added to the "Releasing" commit just cannot be right.

comment:8 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:9 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:10 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:11 in reply to: ↑ 7 Changed 2 years ago by tlucas

Replying to trev:

I'd suggest a hard abort for uncommitted changes. While in the other scenarios I can imagine answering "yes, continue with the release" occasionally, uncommitted changes which will end up being added to the "Releasing" commit just cannot be right.

I agree, changed the description accordingly

comment:12 Changed 2 years ago by tlucas

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:13 Changed 2 years ago by kvas

I've been looking at the review for this ticket and I have a proposal about making the three line "scary message" a bit more friendly and helpful:

I know that you took this from the ticket, but I'm wondering if this line and the following one actually add value. Imagine we have some outgoing commits. What the user will see is:

Detected outgoing changesets:

changeset 1
changeset 2

The above error/s has/have been detected within the repositories.
You might want to check whether this is ok or not.
Are you sure about continuing the release-process?

This is a lot of text, but it's not very informative. How about replacing the first two lines of the scary message with something like "If you proceed with the release, they will be included in the release and pushed.":

Detected outgoing changesets:

changeset 1
changeset 2

If you proceed with the release, they will be included in the release and pushed.
Are you sure about continuing the release-process?

What do you guys think?

comment:14 follow-up: Changed 2 years ago by sebastian

Shouldn't any local changes be committed separately (or reverted), anyway? So perhaps it would be simpler to just abort with an error message when detecting local changes, rather than prompting whether to include them into the version bump commit?

comment:15 in reply to: ↑ 14 Changed 2 years ago by tlucas

Replying to sebastian:

Shouldn't any local changes be committed separately (or reverted), anyway? So perhaps it would be simpler to just abort with an error message when detecting local changes, rather than prompting whether to include them into the version bump commit?

Do you mean mean uncommitted changes or any changes (including changesets)?
In case of the latter, this would make the overall issue a little bit easier to fix.

What do you think about Vasily's proposal, regarding the error-message?

comment:16 follow-up: Changed 2 years ago by sebastian

Oh, I was talking about uncommitted changes, mistakenly assuming this is what this issue is about. As for outgoing commits, the warning/prompt Vasily suggested sounds good to me.

However, what would happen with uncommitted changes?

comment:17 in reply to: ↑ 16 Changed 2 years ago by tlucas

Replying to sebastian:

Oh, I was talking about uncommitted changes, mistakenly assuming this is what this issue is about. As for outgoing commits, the warning/prompt Vasily suggested sounds good to me.

Okay, i will implement the message/prompt accordingly.

However, what would happen with uncommitted changes?

Taken from the description:

Any untracked or uncommitted local changes should cause the
release automation to print an error-message and automatically abort the process.

comment:18 Changed 2 years ago by sebastian

I missed that. Sounds good to me.

comment:19 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:20 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4354, 4355 - handle dirty/outdated repos on release

comment:21 Changed 2 years ago by tlucas

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:22 Changed 2 years ago by tlucas

Please note: The patch introduced a regression, which is handled in #5736

Note: See TracTickets for help on using tickets.