bin/update can leave a developer in a bad state

[Fryguy]

As discussed in ManageIQ/miq_bot#357, bin/update can leave a developer in bad state where they may have "oldish" code in ManageIQ/manageiq locally, but then pull in the "new" code from the dependent git repos. That "new" code may be incompatible with the local "oldish" code, leading to errors that just confuse the developer.

We can't force a git pull locally, in particular, if the user has changes locally they are trying to test with. So, the proposal is to emit a warning (credit goes to @jrafanie)

Proposal: Make ManageIQ/manageiq's bin/update print out a nice red warning that your branch is not up to date with master and recommend that you rebase against it. From the plugin perspective, in the bin/update files of the plugin repos, where it sees you are using a symlinked spec/manageiq directory, that can print out a nice red warning that the spec/manageiq is not up to date with master (this should be coded into the generator).

cc @NickLaMuro @jrafanie @bdunne @chrisarcand

[bdunne]

👍 I like the idea, but how do we make it noticeable? Currently the STDOUT of bin/update is ~840 lines long.

[NickLaMuro]

@Fryguy as I started to elude to regarding " @jrafanie idea" in second part of this comment:

ManageIQ/miq_bot#357 (comment)

There are a few things to keep in mind with this approach.

• bin/update is already overloaded with text, and unless this happens "after an error", this will most likely get missed/ignored as well if it just comes after the bundle update step in bin update.
• This only makes sense on master, or branches off master. When you are working off of a branch for older releases (say fine or darga), this warning will just not make sense. Detecting when to emit a warning is non-trival.
• For the love god... if you make this blinking text... I will flip something...

[NickLaMuro]

@bdunne it is ~840 lines of text if you yarn is up to date and you don't have any migrations to run... far worse if that is not the case.

[Fryguy]

• bin/update is already overloaded with text, and unless this happens "after an error", this will most likely get missed/ignored as well if it just comes after the bundle update step in bin update.

I agree. We may have to do something to cut down on some of the "info" messages from yarn.

• This only makes sense on master, or branches off master. When you are working off of a branch for older releases (say fine or darga), this warning will just not make sense. Detecting when to emit a warning is non-trival.

I could see it making sense, if you don't have the latest fine branch locally for some reason. I disagree about the non-triviality...you can git fetch to get the remote, and see if the remote and the local share the tip commit or not...seems pretty straightforward.

• For the love god... if you make this blinking text... I will flip something...

┬─┬ ︵ /(.□. \）

[jrafanie]

bin/update could check if you're out of date with your remote, and fail unless you pass --force or -f. When you're trying to do something and don't want to move your HEAD, just pass -f. It should really yell at you to update to the tip since you're updating dependencies.

[chrisarcand]

you can git fetch to get the remote, and see if the remote and the local share the tip commit or not

Close - you actually want to check that master's HEAD is just a parent of your HEAD (so one can rebase one's work on top of master and be up to date, as an option.

[NickLaMuro]

I disagree about the non-triviality...you can git fetch to get the remote, and see if the remote and the local share the tip commit or not...seems pretty straightforward.

I think you are still missing the point. People are not just going to run bin/update on master, they are going to run it on feature branches as well. So you have to check that the branch you are on is up to date with master, but from where it was branched from.

Every suggest is assuming you are running from the master branch, but rarely are people working from that branch directly.

bin/update could check if you're out of date with your remote ...

Again, this doesn't help. Your remote doesn't matter when it comes to the git gems. They are dependent on ManageIQ/manageiq:master and only that, so checking your remote has no affect, and will provide a false positive.

[chrisarcand]

So you have to check that the branch you are on is up to date with master, but from where it was branched from.

I'm going, again, back to the point of 'you should always just run the latest' (unless you know what you're doing, in which case add a flag and do whatever you want). Since bin/update updates your gems to their latest, you should be expected to have your feature branch up to date as well. This greatly simplifies the solution.

This is a byproduct of having unlocked git-based gems, as I've been saying from the beginning. If you really want things in sync, either version actual gems or start setting refs to lock to in the Gemfile (which introduces it's own set of nightmares, I'm not actually recommending this), and let's not invent our own git-dependency resolution nightmare.

[Fryguy]

you actually want to check that master's HEAD is just a parent of your HEAD

right...thats what I meant by "share the tip commit", but I didn't elaborate on that I meant the remote master's HEAD, so thanks for the clarification. To clarify even further, the script would know which remote, so on any branch off of master it would check the remote master's HEAD, but a branch off of fine, would know to check remote fine's HEAD.

I think you are still missing the point. People are not just going to run bin/update on master, they are going to run it on feature branches as well. > So you have to check that the branch you are on is up to date with master, but from where it was branched from.

That is exactly what I am describing (and which @chrisarcand clarified). If the remote master's HEAD is in your branches' history then you are up-to-date.

[NickLaMuro]

That is exactly what I am describing (and which @chrisarcand clarified). If the remote master's HEAD is in your branches' history then you are up-to-date.

While I missed the intent of what he was saying until you actually made it more clear, it still has two flaws:

• A false positive warning would be raised on any branches that aren't based off the master branch (i.e., bugfixes branches on old releases), turning into ignored noise.
  • I realize this would required what ever to is built to support this to actually be merged (backported) into those branches, and that probably wouldn't happen, but when the G-release is cut, this would be in there, and basically being noise.
• More importantly, with what is described here, this will go of almost 100% of the time bin/update is run.

To clarify the last point, what I mean is even on a slow day, like today, 6 pull requests were merged into master, so except for right after you rebase, chances are the next time you run bin/update (for what ever that reason may be), you will be greeted with this warning. Basically, this will always displayed, and any complicated logic around when to display this message is just going to be an over-engineered solution, and probably ignored anyway.

[Fryguy]

Turns out this is easier than I thought, since it's built-in to git... git merge-base --is-ancestor remotes/upstream/master HEAD tells you if you are up-to-date or not.

[tjyang]

Hi @Fryguy
Would mind to provide exact git command to display if my local master is out of date ?

[root@miq02 vmdb]# git remote -v
origin  https://github.com/ManageIQ/manageiq.git (fetch)
origin  https://github.com/ManageIQ/manageiq.git (push)
[root@miq02 vmdb]# git log -1
commit 5bf92a4f7f8bf33eb2ae160d7519dd842e9c4b81
Merge: 61474a7 c5cba61
Author: Martin Povolny <[email protected]>
Date:   Fri Aug 11 10:10:14 2017 +0200

    Merge pull request #15778 from lpichler/visible_for_current_user

    Replacement for CustomButton#available_for_user
[root@miq02 vmdb]#
[root@miq02 vmdb]# git merge-base --is-ancestor remotes/upstream/master HEAD
fatal: Not a valid object name remotes/upstream/master
[root@miq02 vmdb]#

[jrafanie]

@tjyang since your remote is named origin, you'll need to change the command to this:
git merge-base --is-ancestor remotes/origin/master HEAD

[tjyang]

@jrafanie , thanks for the pointer for correct command. I need to learn git more.

[tjyang]

@jrafanie , sorry to bug you again.

• my current local repo is like this

[root@miq02 vmdb]# git log -1
commit 9d413e2bd2406ad6c6711c02fdd86be8caea4a6a
Merge: 63e85c8 756d5b3
Author: Adam Grare <[email protected]>
Date:   Tue Sep 5 15:21:11 2017 -0400

    Merge pull request #15590 from durandom/orchestrate_destroy

    orchestrate destroy of dependent managers
[root@lmiq02 vmdb]#

• It is a few days behind current 09/06/2017 commit.

• But I am not able to see if my local is outdated, even I switched to use SCL git 2.9.3 version

[root@miq02 vmdb]# git merge-base --is-ancestor remotes/origin/master HEAD
[root@miq02 vmdb]# git --version
git version 2.9.3
[root@miq02 vmdb]# ls -l /usr/bin/git
-rwxr-xr-x. 112 root root 1514952 Mar 23  2016 /usr/bin/git
[root@miq02 vmdb]# /usr/bin/git --version
git version 1.8.3.1
[root@miq02 vmdb]# which git
/opt/rh/rh-git29/root/usr/bin/git
[root@miq02 vmdb]#

• What is your git output example that showing local is outdated ?

[Fryguy]

The exit status will tell you out of date, so echo $?
Also, you must be sure your remotes are up-to-date, so do a git fetch origin beforehand

[Fryguy]

$ git fetch upstream
$ git merge-base --is-ancestor remotes/upstream/master HEAD
$ echo $?
1  # <= means I'm out of date

[tjyang]

@Fryguy . Thank you for you git usage guidance.

Following is the successful log for my environment.

[root@miq02 vmdb]# git fetch origin
remote: Counting objects: 72, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 72 (delta 46), reused 48 (delta 46), pack-reused 22
Unpacking objects: 100% (72/72), done.
From https://github.com/ManageIQ/manageiq
   9d413e2..9721f9a  master     -> origin/master
[root@miq02 vmdb]# git merge-base --is-ancestor remotes/origin/master HEAD;echo $?
1
[root@miq02 vmdb]#

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[JPrause]

@Fryguy is this still a valid issue. If not can you close.
If there's no update by next week, I'll be closing this issue.

[JPrause]

Closing issue. If you feel the issue needs to remain open, please let me know and it will be reopened.
@miq-bot close_issue