Skip to content

feat(#1782): documentation for cht-sync v2#1783

Merged
witash merged 10 commits intomainfrom
1782-cht-sync-v2
Apr 10, 2025
Merged

feat(#1782): documentation for cht-sync v2#1783
witash merged 10 commits intomainfrom
1782-cht-sync-v2

Conversation

@witash
Copy link
Copy Markdown
Contributor

@witash witash commented Feb 28, 2025

#1782

Docs for upcoming CHT Sync v2 release. This PR:

  • adds setup info for V2
  • adds setup info for local DBT models (instead of remote files checked into git)
  • covers new compose deployments using profiles

When following setup steps in this docs PR, be sure your CHT Sync repo is on the v2 branch per this PR. That will make sure the --profile logic is added to the compose files and the .env created from the example on has the new DBT_LOCAL_PATH value.

@witash witash requested review from jkuester and mrjones-plip March 3, 2025 10:53
@mrjones-plip
Copy link
Copy Markdown
Contributor

Awesome! I'm excited to dive into this.

Please take a moment to fill out the the ticket description to add a brief description of your changes and anything you'd like to draw attention to.

Copy link
Copy Markdown
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

Yay! So cool to seem more developer friendly changes along with what look to be real world experiences on tuning be codified in our docs - thank you!

See my requests below, but also I think it'd be nice to move the two near identical tuning sections to it's own page maybe? I don't feel too strongly, but we have slightly different wording in k8s vs docker on how to use threads and concurrency, and now need to keep them in sync when updating them.

maybe put all the tuning info on their own page and then add a "to configure in: k8s | docker" tab selection like we do in in the dev node setup section?

@mrjones-plip
Copy link
Copy Markdown
Contributor

Oh yes! If we're going to deprecate the extra docker compose files with the awesome --profile calls (love it!) maybe remove the now unused compose files from CHT Sync repo?

Co-authored-by: mrjones <8253488+mrjones-plip@users.noreply.github.com>
@mrjones-plip
Copy link
Copy Markdown
Contributor

Oh yes! If we're going to deprecate the extra docker compose files with the awesome --profile calls (love it!) maybe remove the now unused compose files from CHT Sync repo?

Ah - I've just discovered the v2 branch in CHT Sync which indeed has the files deleted. Disregard my request!

I'm being a bit passive aggressive and fleshing out the body of this PR - feel free blow away with more current/accurate info!

Copy link
Copy Markdown
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

Ok! I took a little time to do some testing, got really stuck, and then discovered the v2 branch and got mostly unstuck 😅

I made some inline suggestions, but it's not clear how to set up the local dbt model directory. I got the local profile to work by just pointing it to a local git clone of the CHT Pipeline repo, but I think this isn't correct. I should have done a dbt init and then "add cht-pipeline as a dependency" per the docs, right?

I'm not quite sure how to weave it in (a bit of a chicken and egg here), but we should some how:

  • add dbt as a prerequisite for development (for dbt init etc)
  • give a sentence or two about what should be in the DBT_LOCAL_PATH directory

witash and others added 2 commits March 5, 2025 14:36
Co-authored-by: mrjones <8253488+mrjones-plip@users.noreply.github.com>
@witash
Copy link
Copy Markdown
Contributor Author

witash commented Mar 5, 2025

I think it'd be nice to move the two near identical tuning sections to it's own page maybe? I don't feel too strongly, but we have slightly different wording in k8s vs docker on how to use threads and concurrency, and now need to keep them in sync when updating them.

added a new page 'Tuning dbt' and linked the two setup pages to it

@witash
Copy link
Copy Markdown
Contributor Author

witash commented Mar 5, 2025

I made some inline suggestions, but it's not clear how to set up the local dbt model directory. I got the local profile to work by just pointing it to a local git clone of the CHT Pipeline repo, but I think this isn't correct. I should have done a dbt init and then "add cht-pipeline as a dependency" per the docs, right?

I'm not quite sure how to weave it in (a bit of a chicken and egg here), but we should some how:

* add `dbt` as a prerequisite for development (for `dbt init` etc)

* give a sentence or two about what should be in the `DBT_LOCAL_PATH` directory

The thing with the local profile is it uses a volume with a path on the host and there's not a good way to set a default path or anything; file paths in docker compose must be absolute. So DBT_LOCAL_PATH, and a local dbt project, is required. It makes it a little harder to just get a local copy of cht-sync running without any custom models, because you can't just use the remote cht-pipeline repo with CHT_PIPELINE_BRANCH_URL. It needs at least an empty project.

So I added "a dbt project" to prerequisites and changed wording to make this more clear.

@mrjones-plip
Copy link
Copy Markdown
Contributor

Thanks for implementing suggestions! I'll take another pass at trying to deploy locally today or tomorrow.

I realize we don't cover the upgrade from v1 to v2. And...there's breaking changes, hence the major SemVer bump, yeah? Off the top of my head I can think of:

  • compose files being deleted
  • .env var variables being added
  • compose signatures changing (eg --profile production)

but I haven't read into a lot/most of the under the hood changes - these are breaking? We for sure should add an upgrade steps, and possibly list out what the breaking changes are?

Copy link
Copy Markdown
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Very nice! I def agree with mrjones that we should have at least some kind of migration guide for how to get from v1 to v2.

Copy link
Copy Markdown
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

Great to see the comments on v1 -> v2 updating - thank you!

Do we want to bump the schema? I don't know how this is used so maybe not?

Also - so sorry for the delays in timely responses :(

I think this is close though :) !!

@witash witash requested review from jkuester and mrjones-plip March 17, 2025 13:10
Copy link
Copy Markdown
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

🍇

@witash
Copy link
Copy Markdown
Contributor Author

witash commented Mar 28, 2025

@mrjones-plip missed your last question, can you re-review so this can be merged?

Do we want to bump the schema? I don't know how this is used so maybe not?

Its not very well defined what this version is, and is just a default suggestion anyway. But to the extent that it is defined, its intended as the version of the data, not the code, so I don't think it should change here.

Copy link
Copy Markdown
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

What?! This was blocked on me this whole time 😭 - I'm sorry!

Let's ship this and fix anything folks find later - lgtm!

@witash witash merged commit 5b20cc3 into main Apr 10, 2025
2 checks passed
@witash witash deleted the 1782-cht-sync-v2 branch April 10, 2025 07:31
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

Successfully merging this pull request may close these issues.

3 participants