Skip to content

Syncer docs: adds a guide on running the syncer locally and updates slightly outdated doc #1775

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 2, 2022
Merged

Syncer docs: adds a guide on running the syncer locally and updates slightly outdated doc #1775

merged 1 commit into from
Sep 2, 2022

Conversation

m1kola
Copy link

@m1kola m1kola commented Aug 16, 2022

Summary

  • Updates instructions on how to enable a syncer on a physical cluster.
  • Updates development guide on how to build a syncer image and run syncer in a kind cluster with a local registry.
  • Adds a guide on how to run a syncer locally for development.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2022

Hi @m1kola. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sttts sttts requested a review from s-urbaniak August 16, 2022 22:45
m1kola
m1kola commented Aug 16, 2022
View reviewed changes
Updates syncer doc
e395ce2 
* Updates instructions on how to enable a syncer on a physical cluster.
* Updates development guide on how to build a syncer image
  and run syncer in a kind cluster with a local registry.
* Adds a guide on how to run a syncer locally for development.
@m1kola m1kola force-pushed the update_syncer_dev_docs branch from 6b3dd84 to e395ce2 Compare August 16, 2022 22:57
@stevekuznetsov
Copy link
Contributor

Isn't this going to conflict with #1746 ?

@stevekuznetsov
Copy link
Contributor

We should try to make this in sync with the Makefile parts of kcp-dev/controller-runtime-example#19

@m1kola
Copy link
Author

m1kola commented Aug 17, 2022

Isn't this going to conflict with #1746 ?

It did conflict. I incorporated changes from this PR here too - updated the snippets with kubectl kcp workspace create output and moved deployment creation into the "Running a workload" section.

We should try to make this in sync with the Makefile parts of kcp-dev/controller-runtime-example#19

Could you elaborate please? I'm not sure what I need to do with regards to this.

@stevekuznetsov
Copy link
Contributor

I wonder if there's some way to provide the simple(ish) Makefile (that we will be validating forever) in the doc, maybe in addition to some of the manual lists? I worry otherwise that this version of the doc (which is very detailed and awesome!) will also become stale in a month or two :|

@m1kola
Copy link
Author

m1kola commented Aug 22, 2022

@stevekuznetsov I think having development workflow automated is a good idea. I'm not sure if it is reasonable to automate user part of the doc: as an outsider one might want to know how to sync the clusters without having to read makefile.

I'll be back to compute on the 7th of September. Happy to catch up on this topic after that date.

I worry otherwise that this version of the doc (which is very detailed and awesome!) will also become stale in a month or two :|

It is the nature of the docs :)

stevekuznetsov
stevekuznetsov reviewed Aug 22, 2022
View reviewed changes
Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Content of the docs looks really good! Just two small comments.

docs/syncer.md

You can run the syncer in a kind cluster for development.

1. Create a `kind` cluster with a local registry to simplify syncer development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the registry for this? I was able to build the image locally and use kind load docker-image to put it on the nodes, as long as you're not using a :latest tag it should go straight to storage & not try to pull.

docs/syncer.md
```sh
$ kubectl wait --for=condition=Ready synctarget/<mycluster>
```
1. Create an organisation and immediately enter it:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this does not have to be an org

@sttts
Copy link
Member

sttts commented Sep 2, 2022

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 2, 2022
@sttts
Copy link
Member

sttts commented Sep 2, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022
@sttts
Copy link
Member

sttts commented Sep 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit a0349e5 into kcp-dev:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@davidfestal davidfestal Awaiting requested review from davidfestal

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m1kola @stevekuznetsov @sttts @openshift-merge-robot