update rebase docs to reflect monorepo #3725
Conversation
kcp-ci-bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
size/L
On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>
SimonTheLeg
force-pushed
the
update-rebase-docs
branch
from
299b8d0 to
938b526
Compare
SimonTheLeg
changed the title
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
|
/hold want to collect input from multiple reviewers |
kcp-ci-bot
added
the
do-not-merge/hold
|
/assign |
| # 4. Update kcp-dev/kubernetes | ||
| ## Terminology | ||
| 2. If there were any substantive changes, make the corresponding edits to our generators. |
There was a problem hiding this comment.
We could mention here that vlidation-gen is not applicable because it generates validation code for core types and isn't usable for custom resources at the moment.
Just to avoid confusing someone new whos never looked at the code-gen stuff.
| ### 2.5 Get it merged | ||
| Create your PR, check that CI is passing and get it merged. It is paramount that you do not start with the next step in this guide until your PR is merged! |
There was a problem hiding this comment.
I disagree. I think it's more important to verify that everything is working before making these PRs because if something doesn't work you'll need to setup local redirects for everything anyhow and you'll end up with multiple PRs in kcp-dev/kcp to fix what broke.
Additionally since this applies to the monorepo it might even break things for other developers because the rebase is incomplete.
There was a problem hiding this comment.
For that reason I modified pin-dependency to support local paths, so the kubernetes fork can point to the local versions (that being said the commit adding this functionality must be cherry-picked in the kube fork before using it to point to the local versions^^)
| - Check the changes with `list_changed_files` and `view_changed_files` | ||
| (see above). | ||
|
|
||
| TODO @ntnn Let's discuss how/if we want to put in these two paragraphs | ||
| ```txt | ||
| If a commit applied clean and `list_changed_files` shows no output, you | ||
| can move on to the next commit because there were no changes to the | ||
| files that commit touched between the two kube versions. | ||
| Additionally `view_changed_files` only shows the changes of the | ||
| currently cherry-picked commit to files that were changed between the | ||
| two kube versions. | ||
| ``` |
There was a problem hiding this comment.
list_changed_files and view_changed_files are working off of the premise that the rebaser is cherry-picking the commits one by one.
I don't think this works with an interactive --onto rebase. The problem with the direct rebase is that it assumes that when a patch applied clean that it's fine and moves to the next commit until a commit doesn't apply cleanly.
The thing with the rebase though is that its possible that a function that was modified by us was changed upstream without interfering with our patch.
E.g. I remember that in one file an informer was wrapped in an upstream change and the patch applied clean but that lead to a failure because our patch used the unwrapped informer.
For these cases it makes it vastly easier to cherry-pick one by one and to inspect the changes using those function to see if there was anything changed at all.
|
|
||
| This can delete the `zz_generated.validations.go` - this is expected | ||
| as we drop the validation-gen generator. See | ||
| <https://github.com/kcp-dev/kcp/issues/3562a> for details. |
There was a problem hiding this comment.
| <https://github.com/kcp-dev/kcp/issues/3562a> for details. | |
| <https://github.com/kcp-dev/kcp/issues/3562> for details. |
| -exec sed -e "/k8s.io/ s#v$NEW_VERSION0#v0.0.0#" -i "{}" \; | ||
| ``` | ||
|
|
||
| TODO @ntnn: I think the previous update vendor step already does this. But I was not sure if it is safe to run the sed before running update-codegen. Wdyt? |
There was a problem hiding this comment.
Could be I think the codegen might also fiddle with it? Could leave a comment that it might not be needed and to remove it in future versions of the document.
| ```sh | ||
| # Change KUBE per your local setup | ||
| KUBE=../../../go/src/k8s.io/kubernetes | ||
| gsed -i "s,k8s.io/\(.*\) => .*/kubernetes/.*,k8s.io/\1 => $KUBE/vendor/k8s.io/\1,;s,k8s.io/kubernetes => .*,k8s.io/kubernetes => $KUBE," go.mod | ||
| ``` |
There was a problem hiding this comment.
Might be worth to update the bump-k8s.sh script to support both remote and local rewrites?
| 7. Keep iterating to get all the code to compile | ||
| 8. Get the `lint` and `test` make targets to pass | ||
| 9. Get the `e2e-*` make targets to pass. | ||
| 9. Get the `test-e2e` make targets to pass. |
There was a problem hiding this comment.
| 9. Get the `test-e2e` make targets to pass. | |
| 9. Get the `test-integration` make target to pass | |
| 10. Get the `test-e2e` make target to pass. |
3 participants
This PR updates our rebase docs to be more consistent by using variables allowing for copy-paste commands as well as updating our section on the monorepo.
Summary
What Type of PR Is This?
/kind documentation
Related Issue(s)
Fixes #
Release Notes