Page MenuHomePhabricator
Differential D3124

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

Authored by • max on Feb 7 2022, 6:28 AM.
Tags
None
Referenced Files
F1778978: D3124.id9609.diff
Fri, May 17, 4:55 PM
F1778977: D3124.id9580.diff
Fri, May 17, 4:54 PM
F1778976: D3124.id9509.diff
Fri, May 17, 4:54 PM
F1778975: D3124.id9421.diff
Fri, May 17, 4:54 PM
F1778974: D3124.id9347.diff
Fri, May 17, 4:54 PM
F1778973: D3124.id.diff
Fri, May 17, 4:53 PM
F1778972: D3124.diff
Fri, May 17, 4:53 PM
Unknown Object (File)
Fri, May 10, 11:10 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
• karol
tomek

Details

Reviewers
tomek
varun
• karol
ashoat
Commits
rCOMM513fed08000c: [services][refactoring] Tunnelbroker - Switch from 'Id' to 'ID' in the codebase.
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• max created this revision.Feb 7 2022, 6:28 AM
Herald added subscribers: • karol, atul, • adrian and 2 others. · View Herald TranscriptFeb 7 2022, 6:28 AM
• max edited the summary of this revision. (Show Details)Feb 7 2022, 6:29 AM
• max added reviewers: tomek, varun.
• max edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B6594: Diff 9347.Feb 7 2022, 6:33 AM
• max requested review of this revision.Feb 7 2022, 6:33 AM
Herald added a reviewer: • karol. · View Herald TranscriptFeb 7 2022, 6:33 AM
• karol requested changes to this revision.Feb 7 2022, 7:29 AM

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
• max planned changes to this revision.Feb 7 2022, 7:44 AM
• max updated this revision to Diff 9421.Feb 8 2022, 4:02 PM

Rebase.

• max added a comment.Feb 8 2022, 4:04 PM
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.

Harbormaster failed remote builds in B6655: Diff 9421!Feb 8 2022, 4:05 PM
• max updated this revision to Diff 9422.Feb 8 2022, 4:36 PM

Fixing nit.

Harbormaster failed remote builds in B6656: Diff 9422!Feb 8 2022, 4:40 PM
• max mentioned this in D3113: [services] Tunnelbroker - Requests SessionID format validation..Feb 8 2022, 5:57 PM
• max mentioned this in D3103: [services] Tunnelbroker - Handle SessionID collision without erroring, using random device to generate it..Feb 8 2022, 6:20 PM
• max added a child revision: D3103: [services] Tunnelbroker - Handle SessionID collision without erroring, using random device to generate it..Feb 8 2022, 6:26 PM
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
• karol accepted this revision.Feb 9 2022, 4:26 AM

Thanks for rebasing, looks alright

• max planned changes to this revision.Feb 9 2022, 2:29 PM
• max added a child revision: D3157: [services][refactoring] Tunnelbroker - AMQP Manager refactoring into a class..Feb 9 2022, 11:14 PM
• max updated this revision to Diff 9509.Feb 9 2022, 11:32 PM

Remove table names changes.

Harbormaster failed remote builds in B6702: Diff 9509!Feb 9 2022, 11:36 PM
• max marked an inline comment as done.Feb 9 2022, 11:40 PM
• 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.

tomek accepted this revision.Feb 10 2022, 3:25 AM

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
Herald added a reviewer: ashoat. · View Herald TranscriptFeb 10 2022, 3:25 AM
Herald added a subscriber: • benschac. · View Herald Transcript
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 planned changes to this revision.Feb 11 2022, 9:15 AM
• max marked an inline comment as done.
• max updated this revision to Diff 9580.Feb 11 2022, 12:53 PM

Rebase.

• max added a child revision: D3113: [services] Tunnelbroker - Requests SessionID format validation..Feb 11 2022, 1:00 PM
Harbormaster completed remote builds in B6759: Diff 9580.Feb 11 2022, 1:00 PM
• max added a comment.Feb 11 2022, 1:04 PM
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 marked an inline comment as done.Feb 11 2022, 1:05 PM
• 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.

ashoat accepted this revision.Feb 13 2022, 9:05 PM

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
• max marked an inline comment as done.Feb 14 2022, 1:10 AM
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.

• max updated this revision to Diff 9609.Feb 14 2022, 5:32 AM

Rebase.

Harbormaster completed remote builds in B6787: Diff 9609.Feb 14 2022, 5:37 AM
Closed by commit rCOMM513fed08000c: [services][refactoring] Tunnelbroker - Switch from 'Id' to 'ID' in the codebase. (authored by • max). · Explain WhyFeb 14 2022, 5:43 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMM513fed08000c: [services][refactoring] Tunnelbroker - Switch from 'Id' to 'ID' in the codebase..

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
docker-server/
contents/
server/
src/
Amqp/
16 lines
Database/
8 lines
28 lines
12 lines
24 lines
6 lines
14 lines
6 lines
14 lines
Service/
70 lines
Tools/
2 lines
12 lines

Diff 9610

View Options

services/tunnelbroker/docker-server/contents/server/src/Amqp/AmqpManager.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.h

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.h

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/DeviceSessionItem.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/PublicKeyItem.h

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/PublicKeyItem.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/SessionSignItem.h

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Database/SessionSignItem.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Service/TunnelbrokerServiceImpl.cpp

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.h

Loading...
View Options

services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.cpp

Loading...