Android: Brainstorm how to prevent lib branch merged too early

[tobiasKaminsky]

Sometimes we have change that requires a PR both on client and library.

If we merge library PR too early, then every other PR on client will fail until we merge client PR (given that we have a breaking change, e.g. rename of a class).

On client our drone system checks if library is set to master, else it will refuse to merge.
On library PR we can always merge it directly and thus break it.

We would need some way to make sure that both PRs are ready and merge-able and only then allow them to merge them within a short time span.

On client side we are somewhat safe, as this PR can only be merged once we switch to master library branch.

So the only thing is how to prevent that we merge library PR too early.

My idea is:

• add "Needed by #LinkToPRonClientRepo
• drone will check this and then check if the client PR is approved and all checks are passed and only then will allow it to merge.

Drawback: we have to remember to add the keyword to PR description.

Do you think this will work?
@AndyScherzinger @ezaquarii

[AndyScherzinger]

any kind of check would be nice while I haven't understood how we would block a lib-PR from being merged breaking the app-master (or any other branch/PR)

[tobiasKaminsky]

1. pin the library version on app master to some rc or daily build that works

This would then mean a change every time we change library or we sometimes might get out of sync

1. merge app and library into a monorepo that would be kept in sync and build library out of it

This would revert our idea with having them independant.

1. keep the snapshot, but still provide timestamped library builds, so I can switch to something that works for me in case of breakage

This is already possible as we use jitpack: https://jitpack.io/#nextcloud/android-library
There you can browse any recent build and on "get it" you see how to integrate it.

1. use git submodules and default to source library

How would this help? git submodule or jitpack do the same, if we target them to latest master.

I would go for "my" idea above as this does not change anything in current workflow.

[tobiasKaminsky]

any kind of check would be nice while I haven't understood how we would block a lib-PR from being merged breaking the app-master (or any other branch/PR)

This will also not work reliable.

After thinking about it again and again, Chris' idea

1. pin the library version on app master to some rc or daily build that works

might be the only one working:

• for master client branch we switch from -SNAPSHOT to buildId, e.g. 3652654454
• this ensures that master will always work

When creating a new feature that needs client/lib change:

• create lib PR -> will create jitpack buildId
• use this in client PR
• downside: if change to lib is needed, also a change on client PR is needed
• if everything is fine we can merge library PR at any time (as client master is still set to old revision)
• once client PR is fine, we merge this with the exact revision library

--> everything fine

Upon preparing a release, we can tag one of the commits.

[tobiasKaminsky]

@ezaquarii @AndyScherzinger ^

[AndyScherzinger]

I think it has to stay as is, else all branches will have to be updated with new Lib builds again and again.

Lib PRs should be flagged for API breaks which can't be merged early but have to be merged in sync with the corresponding client PR.

[ezaquarii]

This looks like least friction solution for me and would probably work with widest audience.

I'd be also happy to keep the snapshot in gradle if that helps devs to catch issues with library early, if some easy fallback to last working lib version is possible in case of breakage. That would probably require a daily/on commit push of the lib artifact.

[tobiasKaminsky]

Then let us go with

• breaking library PRs have a label "client change required"
• in first commit we should link to corresponding PR on client
• this will prevent GitHub from merging these PRs
• on client side: PR cannot be merged as it library branch is not set to master

Upon merge:

• remove label on library PR
• merge it
• change branch on client PR and merge it
  -> still we will have a little time frame where master cannot be built
  (depending on CI it should be rather small)

EDIT: I'll try this with https://github.com/nextcloud/android-library/pull/324

[AndyScherzinger]

this will prevent GitHub from merging these PRs

What is needed for this to work? (never knew one could block a merge with a commit comment.)

[tobiasKaminsky]

this will prevent GitHub from merging these PRs

What is needed for this to work? (never knew one could block a merge with a commit comment.)

There is an app for this: https://github.com/Muhnad/dont-merge
But I failed to install it.

So for now we only have the label.
I could also do it via drone, but this means that we would have to re-trigger drone just to get rid of this warning.

[AndyScherzinger]

So for now we only have the label.

imho: Good enough! 👍

[tobiasKaminsky]

I have created a custom webhook, which

• acts on label change
• check if "client change required" label is set
• if set, prevents
  
• else
  

[tobiasKaminsky]

So, we now "only" have to remember to set the label.
I think this is safe enough.

[tobiasKaminsky]

Doc is in https://github.com/nextcloud/android/pull/4295