Skip to content
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.

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

The banishment of Address #1880

Merged
merged 31 commits into from
Oct 23, 2024
Merged

The banishment of Address #1880

merged 31 commits into from
Oct 23, 2024

Conversation

cloutiertyler
Copy link
Contributor

@cloutiertyler cloutiertyler commented Oct 19, 2024

Description of Changes

Jeff and I worked on Friday to remove all uses of Address as an identifier for a database. Address is still used for what will eventually become ConnectionId.

There is a corresponding private PR.

API and ABI breaking changes

This breaks the CLI API as well as the HTTP and WebSocket APIs. It also edits the system tables.

Expected complexity level and risk

4

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

This impacts many APIs as mentioned above. The change is mostly mechanical, but not entirely.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • We have run the unit tests and smoke tests.

@mamcx
Copy link
Contributor

mamcx commented Oct 21, 2024

lFYI this could conflict and/or get good help from #1876, in special everywhere we pass Address and/or ExecutionContext it will removed it in #1876, ie I several of the same lines doing that refactor.

Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

lgtm

@jsdt jsdt enabled auto-merge October 23, 2024 00:51
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Trusting kim, just in case I somehow wound up as a code owner on this. I have not actually reviewed this PR, but Kim's review satisfies me.

Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

I have not tested this but I've reviewed the files I'm a code-owner of. I left one follow-up note for Zeke but these changes look correct to me.

@jsdt jsdt added this pull request to the merge queue Oct 23, 2024
Merged via the queue into master with commit 83fc5c3 Oct 23, 2024
7 of 8 checks passed
@cloutiertyler
Copy link
Contributor Author

Although I am a partial author of this, I just want to so that this one looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants