Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitting API and data sources #11

Open
defgsus opened this issue May 3, 2022 · 5 comments
Open

Splitting API and data sources #11

defgsus opened this issue May 3, 2022 · 5 comments

Comments

@defgsus
Copy link
Collaborator

defgsus commented May 3, 2022

No description provided.

defgsus added a commit to ParkenDD/ParkAPI2-sources that referenced this issue May 3, 2022
- copied from ParkAPI2, adjusted README.md a bit
defgsus added a commit that referenced this issue May 4, 2022
- add submodules to checkout action
- fix path for data-sources requirements.txt
defgsus added a commit that referenced this issue May 4, 2022
@defgsus
Copy link
Collaborator Author

defgsus commented May 4, 2022

Heyho @jklmnn and @kiliankoe

The data-sources are now compartmentalized into ParkAPI2-sources and the CI script and README are updated.

As it looks, you need to review and accept the pull request #12

(well, as admin i can override the review requirement but then again: you need to deploy it on the server so i keep you in the loop. Please check the README about the git submodule steps)

@defgsus
Copy link
Collaborator Author

defgsus commented May 10, 2022

@jklmnn, if you have time please configure this repo the same way as the original ParkAPI, meaning: do not allow merge requests to not interfere with rebasing and please explain how an actual contribution from a fork must be requested.
The last time you picked the commits from a pull requests and did something. I'd like to add this part to the README

@jklmnn
Copy link
Collaborator

jklmnn commented May 16, 2022

I configure the repo. Now PRs can only rebased and merge commits are forbidden. Also all conversations in reviews have to be resolved.
The rationale for not having merge commits is that they infer with rebasing. A linear history allows to rebase the whole branch. Having a merge commit will break that since it has to be reapplied manually each time a rebase is done. This is especially useful when git bisect is used to analyze an old bug but an early commit has to be modified to e.g. adapt to new tooling. Having merge commits basically makes this impossible (or at least utterly annoying).

The review process I have in mind works as follows:
The reviewers will open comments with questions, change requests, etc. The author will have to address all each comment, either with an explanation or justification or with a new commit. When the author has addressed all comments they rerequest the review from the reviewer. The reviewer will see the comments and either continue the discussion or resolve the comment. The author of the commit MUST NOT resolve any comment not written by themselves. This ensures that no review comment is missed.
If the author of the PR has merge rights it's the authors task to merge the commit, if not the maintainer will do that (additionally we could ask the author for confirmation that the PR is ready). This ensures that no PR is merged in an incomplete state.

About the commit picking: Many people just clone the repo, add something to their master/main and then open a PR. That works for the first time but will likely break the second time when the fork is not properly updated. In the particular cases people merged the projects current master into their own master creating a merge commit. Since merge commits are not allowed I cannot directly merge that PR. If it was allowed the projects commit history would include multiple merge commits of other people updating their branches, adding a lot of clutter to our history and making it hard to read.
To fix this I usually open a new branch that is based on the current master, cherry-pick the relevant changes onto that branch and then rebase that branch into master (either by opening a new PR or just on the command line).

The proper way to update a fork is as follows:

$ git remote add upstream <url of the upstream repo>
$ git checkout master
$ git rebase upstream/master
$ # if that fails:
$ git reset --hard upstream/master
$ # note that this will delete any change on master that is different from upstream

Then all new changes can be based on the proper master branch.

A good example for the problem that merge commits create is offenesdresden/ParkAPI#235. This PR would add 19 commits. 8 of them are merge commits, 10 of them are commits that are already on master. There is only a single commit that contains the change this PR intends to add to the upstream repo.

Just tell me if you need any additional info.

@defgsus
Copy link
Collaborator Author

defgsus commented May 19, 2022

Hey, thanks a lot. I'm still trying to understand the work-around. The appearance of the useless merge commits is understood.

If a contributor forks the repo, commits something and then later (after new upstream commits) pulls with git pull --rebase, these useless merge commits would not happen, right? Well, unless there's a merge conflict.

I start to get an idea of the git rebase upstream/master part above. As the docs say: The changes will be boiled down to the set of commits that would be displayed with git log <upstream>..HEAD. But i guess your if that fails comment addresses the same problem of not resolvable merge conflicts.
(in which case a git stash might help?)

@jklmnn
Copy link
Collaborator

jklmnn commented Jun 2, 2022

Sorry for the late reply. I'm not 100% sure I understand your question correctly.
What git rebase does is basically taking your HEAD and apply the commits from HEAD .. upstream HEAD to your local branch. For this to work git needs to find a compatible set of changes. It also works if you have different commit IDs. So if you have branch A and B. They're identical. Then you cherry pick a single commit to B. git rebase B on A will then apply that commit with the same commit ID to A. If you instead cherry pick the commit from C to A both A and B will have the same set of changes but different commit IDs. If you run git rebase B now A will be identical to B again, because git recognizes that this is the same change.
If you cherry pick two different commits to A and B git rebase B will try to use B as your new "base" which means that it will take the common history of A and B and then apply the commit from B first and the commit from A second. So A contains both commits after that with the HEAD of A being the same commit after the operation. If it cannot apply the changes made on A on the commits of B, the rebase will stop with a merge conflict that you have to resolve by hand. If you want to do this depends on your situation. If you have changes on A that are older but you need them on the newer version (B) you have to resolve the merge conflict by hand. If you just want to have the same version as B you'd just use git reset --hard B which is basically telling git to throw everything on the current branch away and use the version of B.
I hope that answers your questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants