Page MenuHomePhabricator

[services][refactoring] Tunnelbroker - Switch from 'Id' to 'ID' in the codebase.
ClosedPublic

Authored by max on Feb 7 2022, 6:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 24, 11:57 PM
Unknown Object (File)
Thu, Oct 24, 11:57 PM
Unknown Object (File)
Thu, Oct 24, 11:57 PM
Unknown Object (File)
Thu, Oct 24, 11:57 PM
Unknown Object (File)
Thu, Oct 24, 11:56 PM
Unknown Object (File)
Thu, Oct 24, 11:56 PM
Unknown Object (File)
Thu, Oct 17, 11:45 PM
Unknown Object (File)
Thu, Oct 17, 11:45 PM

Details

Summary

Refactoring codebase to switch from ...Id to ...ID in the variables, methods, functions.

Linear task: ENG-617

Test Plan

Successfully built tunnelbroker server after refactoring.

Diff Detail

Repository
rCOMM Comm
Branch
IdToID
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

max added reviewers: tomek, varun.
max edited the summary of this revision. (Show Details)

I'm not sure what is wrong here... I cannot patch this diff, @geekbrother can you make it patchable? Maybe rebase it to master or something? For diffs like this where there isn't really a logic to verify, it's very useful to be able to patch it to the local environment and use the local IDE to analyze the changes.

This revision now requires changes to proceed.Feb 7 2022, 7:29 AM
In D3124#82310, @karol-bisztyga wrote:

I'm not sure what is wrong here... I cannot patch this diff, @geekbrother can you make it patchable? Maybe rebase it to master or something? For diffs like this where there isn't really a logic to verify, it's very useful to be able to patch it to the local environment and use the local IDE to analyze the changes.

Rebased on the master. Now it must work.

tomek requested changes to this revision.Feb 9 2022, 3:51 AM
tomek added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.cpp
8–9 ↗(On Diff #9422)

Can we update these without any migration on the working instance? Do we need to clean something up on the instances?

This revision now requires changes to proceed.Feb 9 2022, 3:51 AM

Thanks for rebasing, looks alright

Remove table names changes.

max added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.cpp
8–9 ↗(On Diff #9422)

Can we update these without any migration on the working instance? Do we need to clean something up on the instances?

Good catch @palys-swm ! I've removed changes of the table names and created a separate task as we need to consider the migration procedure for that.

This build fails, but my guess is that rebasing on master should fix the issue

services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.cpp
8–9 ↗(On Diff #9422)

We don't necessarily need a migration. We can probably just recreate a couple of things, as we probably don't store anything important there yet - but we need to think about what needs to be done and cleaned.

This revision is now accepted and ready to land.Feb 10 2022, 3:25 AM
This revision now requires review to proceed.Feb 10 2022, 3:25 AM
ashoat requested changes to this revision.Feb 10 2022, 5:51 AM

I don't understand why the build failed. It looks like the CI is green, and @geekbrother recently rebased. Is it possible that this diff is leading to a build failure? @geekbrother, can you look into it?

This revision now requires changes to proceed.Feb 10 2022, 5:51 AM
max marked an inline comment as done.
In D3124#83888, @ashoat wrote:

I don't understand why the build failed. It looks like the CI is green, and @geekbrother recently rebased. Is it possible that this diff is leading to a build failure? @geekbrother, can you look into it?

I've rechecked it before it builds locally.
The build success now after rebasing.
Not sure it was the CI, other services, or rebasing issue, but it is successful now.

max added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.cpp
8–9 ↗(On Diff #9422)

We don't necessarily need a migration. We can probably just recreate a couple of things, as we probably don't store anything important there yet - but we need to think about what needs to be done and cleaned.

Yes, I think this is for another small diff and tracked by this task.

Didn't have time to actually review it... assuming @geekbrother has been thorough in checking all of the sites. Might be good to grep around for places to migrate one last time after rebasing and before landing!

This revision is now accepted and ready to land.Feb 13 2022, 9:05 PM
In D3124#84774, @ashoat wrote:

Didn't have time to actually review it... assuming @geekbrother has been thorough in checking all of the sites. Might be good to grep around for places to migrate one last time after rebasing and before landing!

Yep, I've searched for the all Ids in the codebase to not miss something.