You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Figure out the work needed for Resource Manager service #297
Merge commits into master, but comment on the readme saying "do not use, currently under development".
@aozarov suggests #1 because incremental PRs are much easier to review and also because people using the library won't mistakenly think the Resource Manager library is ready. I agree with those points, but there's a downside that we can't rebase work to make the history clean. (Suppose there's a situation like in with the update-datastore branch in #277, where some of my PRs have been merged into master. Then any PR with a rebase will cause a merge conflict because my commits already upstream will have different commit hashes). I vote option 1 because option 2 seems impractical for the reviewer and option 3 could be confusing to users. What's your opinion/what do you plan to do for BigQuery?
@ajkannan In theory you can rebase even when using option 1) but not through a PR. Let say we have a bigquery branch. Given that you have write access to the gcloud-java repo you can fetch gcloud-java, checkout bigquery, rebase it to master and force push (I am not suggesting this, but it can be done).
An alternative would be creating a bigquery branch in my own fork, let me call it mziccard/bigquery. For every feature I create a branch mziccard/bigquery-feauture-x, once done with the feature I make a PR from mziccard/bigquery-feauture-x into mziccard/bigquery (notice this is local). Every time I want to, I rebase mziccard/bigquery to gcloud-java master (I do no harm as the branch is mine). Once done I rebase mziccard/bigquery once more and do a PR from mziccard/bigquery into gcloud-java/bigquery. This way the history is clean. The "problem" is that the reviewer has to do his job on my local repository, not here.
Doing the code review in my fork would be cleaner in terms of history, but one downside is that we lose the review history. I'm fine both ways too (either the public branch or as feature PRs into my personal fork). @aozarov what's your preference given this additional option?
What about having an incremental PR for gcloud-java that does not get merged until it's ready to use? This way we can rebase the PR as many times as we want and once ready merge it directly into master. This implies
no need for an extra branch
rebase is allowed
reviewer must review on a commit basis rather than on a PR basis (which is a very good practice anyway)
reviewer must review on a commit basis rather than on a PR basis (which is a very good practice anyway)
I think this approach works well for smaller PRs but I have my doubts about big/continues PRs.
Personally I need sometimes to look at the "forest" rather than looking at the "trees", and in this
case the forest is just going to get bigger and bigger.
I imagine that we can be super strict in our commits to make it easier on the reviewer by, e.g., addressing all comments and nothing else in subsequent commits until all comments has been addressed and only then continuing with the next set of functionality, but I doubt that will work well and fear it will
also affect productivity.
Also, how this approach work with parallel efforts (PRs by the same person or others)?
Considering that, my preference is for option (1), however I am willing to try other approaches if you
think they will produce better results.
Activity
ajkannan commentedon Oct 30, 2015
@mziccard I want to get your opinion on a good workflow for this issue. I think there are a couple options:
update-datastorebranch) and merge in incremental PRs.@aozarov suggests #1 because incremental PRs are much easier to review and also because people using the library won't mistakenly think the Resource Manager library is ready. I agree with those points, but there's a downside that we can't rebase work to make the history clean. (Suppose there's a situation like in with the
update-datastorebranch in #277, where some of my PRs have been merged into master. Then any PR with a rebase will cause a merge conflict because my commits already upstream will have different commit hashes). I vote option 1 because option 2 seems impractical for the reviewer and option 3 could be confusing to users. What's your opinion/what do you plan to do for BigQuery?mziccard commentedon Oct 30, 2015
@ajkannan In theory you can rebase even when using option 1) but not through a PR. Let say we have a
bigquerybranch. Given that you have write access to thegcloud-javarepo you can fetchgcloud-java, checkoutbigquery, rebase it to master and forcepush(I am not suggesting this, but it can be done).An alternative would be creating a
bigquerybranch in my own fork, let me call itmziccard/bigquery. For every feature I create a branchmziccard/bigquery-feauture-x, once done with the feature I make a PR frommziccard/bigquery-feauture-xintomziccard/bigquery(notice this is local). Every time I want to, I rebasemziccard/bigquerytogcloud-javamaster (I do no harm as the branch is mine). Once done I rebasemziccard/bigqueryonce more and do a PR frommziccard/bigqueryintogcloud-java/bigquery. This way the history is clean. The "problem" is that the reviewer has to do his job on my local repository, not here.I am fine either way!
ajkannan commentedon Oct 30, 2015
Doing the code review in my fork would be cleaner in terms of history, but one downside is that we lose the review history. I'm fine both ways too (either the public branch or as feature PRs into my personal fork). @aozarov what's your preference given this additional option?
mziccard commentedon Oct 31, 2015
What about having an incremental PR for
gcloud-javathat does not get merged until it's ready to use? This way we can rebase the PR as many times as we want and once ready merge it directly into master. This impliesaozarov commentedon Oct 31, 2015
I think this approach works well for smaller PRs but I have my doubts about big/continues PRs.
Personally I need sometimes to look at the "forest" rather than looking at the "trees", and in this
case the forest is just going to get bigger and bigger.
I imagine that we can be super strict in our commits to make it easier on the reviewer by, e.g., addressing all comments and nothing else in subsequent commits until all comments has been addressed and only then continuing with the next set of functionality, but I doubt that will work well and fear it will
also affect productivity.
Also, how this approach work with parallel efforts (PRs by the same person or others)?
Considering that, my preference is for option (1), however I am willing to try other approaches if you
think they will produce better results.
mziccard commentedon Oct 31, 2015
@aozarov Yes my latest proposal assumes that we are very organized in commit/code/review phases.
The workflow I expect is:
The idea is that basic support for a new service should be built incrementally so other PRs shouldn't be needed.
But hey I am super fine with (1) I just like playing with git :)
ajkannan commentedon Nov 2, 2015
It sounds like 1 works for everyone, so I'll go ahead and make a new branch off master for this issue.
chore: include generated samples dir (#297)
chore: release 0.118.4-SNAPSHOT (googleapis#297)
8 remaining items
chore(main): release 1.0.5 (#300)
chore(main): release 2.2.8-SNAPSHOT (#297)
chore(main): release 1.2.8 (#298)
chore(main): release 2.4.4 (#310)
chore(java): add a note in README for migrated split repos (googleapi…
deps: update actions/checkout action to v3 (#297)
chore(main): release 1.2.8 (#304)
chore(deps): update dependency com.google.cloud:libraries-bom to v13.…
chore: add back blunderbuss configs overridden by yoshi-automation (#297
chore(deps): update dependency com.google.cloud:google-cloud-datastor…
chore(deps): update dependency com.google.cloud:libraries-bom to v13.…
build(java): use actions/github-script v3 tag rather than full semver (…
build(java): use actions/github-script v3 tag rather than full semver (…
docs: remove experimental warning for ordering keys properties. (#297)
docs: remove experimental warning for ordering keys properties. (#297)