Update doc about PR #5756

bmarty · 2022-04-13T13:43:30Z

no need for a changelog file I guess, opened for comment!

Rendered: https://github.com/vector-im/element-android/blob/773b9b7764364acaf889e93a31ec2765c0fd73d3/docs/pull_request.md#base-branch

…ranch content.

ericdecanini · 2022-04-13T13:52:38Z

docs/pull_request.md

+- if a feature implementation is split into multiple PRs. We can have a chain of PRs in this case. PR can be merged one by one on develop, and GitHub change the target branch to `develop` for the next PR automatically.
+- we want to merge a PR from the community, but there is still work to do, and the PR is not updated by the submitter. In this case, we can create a new branch, push it, and change the target branch of the PR to this new branch. The PR can then be merged, and we can add more commits to fix the issues. After that a new PR can be created with `develop` as a target branch.
+
+**Important notice 1:** Releases are created from the `develop` branch. So `develop` branch should always contain a "releasable" source code. So when a feature is being implemented with several PRs, it has to be disabled by default, until the feature is fully implemented. A last PR to enable the feature can then be created.


Perhaps we can explicitly state what's meant by a feature being disabled? Like whether it's via feature flag or being kept on another branch, etc

When multiple developers are working on the same feature, waiting for others' PR to be merged into develop is sometimes blocking/time-consuming. I think we should consider keeping them in another branch and create new branches on top of it instead of develop.

I think working on a different branch for a new feature may be painful concerning resolving conflicts when merging the feature branch into develop: it will raise the same problems as forked project. In my opinion, using a feature flag is the painless approach to smoothly integrate new features. But I admit we should find a good process to be faster to avoid blocks when working on the same feature.

Perhaps we can explicitly state what's meant by a feature being disabled? Like whether it's via feature flag or being kept on another branch, etc

3faf0a2

github-actions · 2022-04-13T14:11:57Z

Unit Test Results

122 files +  8 122 suites +8 2m 18s ⏱️ +44s
205 tests +  4 205 ✔️ +  4 0 💤 ±0 0 ❌ ±0
690 runs +16 690 ✔️ +16 0 💤 ±0 0 ❌ ±0

Results for commit 3faf0a2. ± Comparison against base commit 2761b35.

This pull request removes 10 and adds 14 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given when editing homeserver, then updates selected homeserver state and emits edited event
org.matrix.android.sdk.api.pushrules.PushRuleActionsTest ‑ test_action_parsing
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_displayName_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_cake_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_cakelie_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_path_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_type_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_words_only_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_notice_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_roommember_condition

im.vector.app.features.crypto.UISIDetectorTest ‑ If event decrypted during grace period should not trigger detection
im.vector.app.features.crypto.UISIDetectorTest ‑ trigger detection after grace period
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given in the sign up flow, when editing homeserver errors, then does not update the selected homeserver state and emits error
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given in the sign up flow, when editing homeserver, then updates selected homeserver state and emits edited event
im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
org.matrix.android.sdk.api.session.pushrules.PushRuleActionsTest ‑ test_action_parsing
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_displayName_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_cake_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_cakelie_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_path_condition
…

♻️ This comment has been updated with latest results.

ariskotsomitopoulos · 2022-04-13T17:49:46Z

docs/pull_request.md

+Exceptions can occur:
+
+- if a feature implementation is split into multiple PRs. We can have a chain of PRs in this case. PR can be merged one by one on develop, and GitHub change the target branch to `develop` for the next PR automatically.
+- we want to merge a PR from the community, but there is still work to do, and the PR is not updated by the submitter. In this case, we can create a new branch, push it, and change the target branch of the PR to this new branch. The PR can then be merged, and we can add more commits to fix the issues. After that a new PR can be created with `develop` as a target branch.


I totally agree, we should just find the best process to inform the submitter, maybe via email or with a comment on his PR

bmarty · 2022-05-12T13:02:59Z

Let's merge the change and iterate later if necessary!

github-actions · 2022-05-12T13:41:36Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=15 failures=5 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=9 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

bmarty added 2 commits April 13, 2022 15:34

Add a paragraph about the target branch, and a word about develop b…

e7aaf12

…ranch content.

Add a notice about database migration

773b9b7

ericdecanini reviewed Apr 13, 2022

View reviewed changes

ariskotsomitopoulos reviewed Apr 13, 2022

View reviewed changes

Update after PR reviews

3faf0a2

bmarty merged commit f54c865 into develop May 12, 2022

bmarty deleted the feature/bma/pr_doc_update branch May 12, 2022 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update doc about PR #5756

Update doc about PR #5756

bmarty commented Apr 13, 2022 •

edited

Loading

ericdecanini Apr 13, 2022

onurays Apr 13, 2022

mnaturel Apr 14, 2022

bmarty May 4, 2022

github-actions bot commented Apr 13, 2022 •

edited

Loading

ariskotsomitopoulos Apr 13, 2022

bmarty May 4, 2022

bmarty commented May 12, 2022

github-actions bot commented May 12, 2022

Update doc about PR #5756

Update doc about PR #5756

Conversation

bmarty commented Apr 13, 2022 • edited Loading

ericdecanini Apr 13, 2022

Choose a reason for hiding this comment

onurays Apr 13, 2022

Choose a reason for hiding this comment

mnaturel Apr 14, 2022

Choose a reason for hiding this comment

bmarty May 4, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2022 • edited Loading

Unit Test Results

ariskotsomitopoulos Apr 13, 2022

Choose a reason for hiding this comment

bmarty May 4, 2022

Choose a reason for hiding this comment

bmarty commented May 12, 2022

github-actions bot commented May 12, 2022

Matrix SDK

Integration Tests Results:

bmarty commented Apr 13, 2022 •

edited

Loading

github-actions bot commented Apr 13, 2022 •

edited

Loading