Skip to content

Conversation

@mitchellwrosen
Copy link
Contributor

@mitchellwrosen mitchellwrosen commented Feb 12, 2025

Overview

The PR amends the namespace diff infrastructure to use the 3-way diff algorithm, and adds a new "propagated" diff entry type for propagated updates. It also adds a libdeps diff to the payload.

Test coverage

I've rerun the existing transcripts, which should cover all of the changes. I also amended one to demonstrate that a propagated update gets tagged as such.

Loose ends

One that will be implemented in a follow-on PR:

  • Tagging adds/updates with whether or not they conflict with the merge target branch

@mitchellwrosen mitchellwrosen marked this pull request as ready for review March 3, 2025 16:28
(termNames, typeNames) <- foldMapM namesForReference withPGRefs
pure $ Names.fromTermsAndTypes termNames typeNames
where
-- TODO: Can probably speed this up by skipping suffixification.
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth testing one of the common queries on prod with and without suffixification to see if the real-world cost changes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, how might we go about doing this? (Btw, this was one of the functions you authored before I took over)

Copy link
Member

@ChrisPenner ChrisPenner Mar 3, 2025

Choose a reason for hiding this comment

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

It's a little leg-work to do it, sorry >_>

The queries it's running are here: https://github.com/unisoncomputing/share-api/blob/main/src/Share/Postgres/NameLookups/Queries.hs#L44-L122

You'll need to copy-paste that, then manually fill in the parameters into the query which is a little annoying tbh; you can probably query a release or something from the debug_project_releases view, then cross reference that with scoped_term_name_lookup to get the params you need;

From there you can EXPLAIN ... and/or EXPLAIN ANALYSE on the queries with and without the suffixification.

If you need DB credentials you can get a read-only login for psql via vault read secret/production/enlil/userdb-readonly; For some reason our main DB is still called users 🤷🏼

Or my preferred method:

pgprod_ro () {
        pg_secret=$(vault read /secret/production/enlil/userdb-readonly -format=json | jq .data)
        PGPASSWORD="$(echo "$pg_secret" | jq -r .password)" psql --username "$(echo "$pg_secret"  | jq -r .username)" --host "$(echo "$pg_secret"  | jq -r .hostname)" --port "$(echo "$pg_secret"  | jq -r .port)" -d users "$@"
}

Copy link
Member

@ChrisPenner ChrisPenner Mar 3, 2025

Choose a reason for hiding this comment

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

If you need a hand knowing where to poke around don't hesitate to ask, it's probably good to get more folks used to digging around :)

Copy link
Member

Choose a reason for hiding this comment

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

Are the changes here (and in the other transcript changes) expected? Looks like a conflict was introduced?

Copy link
Member

@ChrisPenner ChrisPenner left a comment

Choose a reason for hiding this comment

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

Looks great, good job on all of this!

I think I'd have expected to see some new transcripts, do the existing ones cover all the new cases w/r to propagated vs manual changes?

Regardless, if the transcript outputs look good we can let @hojberg know about the changes and once he's got a UI ready we can try it out locally and on staging.

Consider it approved ✅, but we should wait till the frontend is ready to handle this version before merging since otherwise it'll block Share deploys until that's done

Also, I see changes in the unison submodule, is there a related PR there that needs to be merged first?

@mitchellwrosen mitchellwrosen requested a review from a team as a code owner March 31, 2025 17:30
@mitchellwrosen mitchellwrosen removed the request for review from a team March 31, 2025 19:30
@ChrisPenner ChrisPenner merged commit 2087e63 into main Apr 3, 2025
8 checks passed
@ChrisPenner ChrisPenner deleted the share-3waydiff branch April 3, 2025 17:21
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