Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Identify trento installation #472

Merged

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Nov 19, 2021

This PR introduces Trento installation identification mechanism

This becomes needed when publishing telemetry data to the telemetry service.

A random UUID is generated once at trento startup and it gets stored in the database for future usage in the system

That value would stay unchanged until the database and specifically that simple information, for any reason, gets cleaned up.
In that case we interpreted the scenario as a new installation.

@nelsonkopliku nelsonkopliku force-pushed the identify_trento_installation branch 2 times, most recently from e32cdc4 to 15bf8bb Compare November 19, 2021 11:39
@dottorblaster
Copy link
Contributor

There are many Trento, but which one is the Trento ™️?

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@nelsonkopliku It looks good. Added some comments and questions before approving hehe

web/app.go Outdated Show resolved Hide resolved
web/app.go Outdated Show resolved Hide resolved
web/app.go Outdated Show resolved Hide resolved
web/services/installation.go Outdated Show resolved Hide resolved
web/services/installation.go Outdated Show resolved Hide resolved
Copy link
Member

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I have some doubts on the naming of the entity/service.
Trento sounds a bit generic and since @dottorblaster is working on something like SystemSettings maybe they could be the same Service/share the same entity?

@nelsonkopliku
Copy link
Member Author

This looks good to me, but I have some doubts on the naming of the entity/service. Trento sounds a bit generic and since @dottorblaster is working on something like SystemSettings maybe they could be the same Service/share the same entity?

@fabriziosestito That's correct what you say, there's a relation with what @dottorblaster is on.
Will do a round of discussion with the team.

@dottorblaster
Copy link
Contributor

Makes me laugh, I was just thinking "how do I manage the ID column", then I saw that basically the Trento ™️ entity overlaps with my SystemSettings work. :D Do you wanna name that so and I rebase against your work @nelsonkopliku?

@nelsonkopliku
Copy link
Member Author

nelsonkopliku commented Nov 19, 2021

Makes me laugh, I was just thinking "how do I manage the ID column", then I saw that basically the Trento tm entity overlaps with my SystemSettings work. :D Do you wanna name that so and I rebase against your work @nelsonkopliku?

@dottorblaster sure thing! So my Trento becomes SystemSettings with an InstallationID?
Good to me! Deal?

@dottorblaster
Copy link
Contributor

@nelsonkopliku

@nelsonkopliku nelsonkopliku force-pushed the identify_trento_installation branch 3 times, most recently from 21675cd to 53a941d Compare November 19, 2021 14:37
@nelsonkopliku nelsonkopliku force-pushed the identify_trento_installation branch 2 times, most recently from e45eb24 to 4eaaa1d Compare November 19, 2021 14:42
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

SMASH THAT MERGE BUTTON

settigsService.On("InitializeIdentifier").Return(uuid.MustParse("59fd8017-b7fd-477b-9ebe-b658c558f3e9"), nil)

return settigsService
}
Copy link
Member

Choose a reason for hiding this comment

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

@dottorblaster take a look, we'll need the same approach for the premium/telemetry middleware, by just adding a On clause to the mock

@nelsonkopliku nelsonkopliku force-pushed the identify_trento_installation branch 2 times, most recently from 5aca739 to 7829bca Compare November 19, 2021 20:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants