adds support for id_column in tables for auto_publish #790
Conversation
|
Oops.🥲I accidentally changed it from a draft state to ready for review. I was on a train |
|
It may take few days for me to complete this. I have converted to draft. |
|
@nyurik Thank you for your guidance. I have made the changes as per your suggestions. Also, I have tested manually with few variations in config files and this works good. Please let me know if anything. I will modify accordingly. |
* don't clone when you can avoid it * i think vec<T> is a bit easier here - just an internal optimization, shouldn't be exposed like this (not a clean external API, but ok internally)
|
I made a few minor suggestions as a pr to your branch. Feel free to merge or comment on it. I'll make a note thorough read tomorrow |
|
@nyurik I have looked and merged the changes. |
* rename configuration `auto_publish.tables.id_format` and `auto_publish.functions.id_format` fields from `id_format` to `source_id_format` fields. The `id_format` will continue to be supported (read) from the configuration, but it will be auto-converted to the new name on save. It is an error to have both in the same config file. * The rename was discussed in #682 * internal refactorings: consolidate PG-related utilities, rename a few vars, move PG errors to their own file. This is partially made due to #790 (thanks @Binabh!) - and should be merged before that to make that PR easier.
|
I see that the renaming is handled at serde serialization level. That is much cleaner approach. If I understand this correctly now the code that I wrote wrote supporting new name could be discarded right? |
|
I will rebase it shortly -- that PR was the result of looking deeply into your code. I should be able to push it to your branch in a bit |
|
I pushed a few minor changes and merged with the main branch. My next todo (i'll fix it) is to make it use the |
Detecting id column should only be done when adding all auto-detected tables to the sources. I rewrote the logic, but did not test it fully. I added a simple test (we may want to add a few more), but currently tests break - not certain why yet - maybe the `from_schema` is not working correctly? It's getting a bit late - try going from here.
|
I made a number of significant changes to the logic of this PR as part of the Binabh#2 (see notes). TODO (if merged):
|
|
I have merged.Can I give TODO list a try? It may take some days though. |
|
Sorry, i just saw your comment - i made a few base improvements in #795 -- that gives some base for testing (I discovered a few minor issues in it). TODO list is still relevant - i will merge the main into your PR once i merge 795, and then won't touch it until you message me :) |
* on `--save-config`, only save configured `auto_publish` settings * alias `from_schemas` as `from_schema` * add integration testing for `auto_publish` * if integration test DB preloading fails, try to clean up the test DB * A few more info traces This change should benefit testing of the #790 cc: @Binabh
Also fixed tests and other minor cleanup
No worries, and thank you for this learning opportunity. I am learning a lot form your commits, commit messages and detailed comments. I will try these TODOs this weekend. |
|
@Binabh let me know if you need any help with this PR, seems like it needs just the tiny push to get ready for merge |
|
@nyurik I have addressed almost all TODOs. Let me know if any improvements are needed. |
|
Looks awesome! I will go over it tonight one last time before merging. I think it would be really good to add one or two more tests, but up to you |
|
@nyurik I have added a test to check for feature id of type bigint. This is my first try writing test in Rust project. So, please let me know about it. I could revert commit if that test is unnecessary. |
Resolves #682