Use v4 instead of v1 uuids#11021
Conversation
ctavan
force-pushed
the
use-v4-instead-of-v1-uuids
branch
from
9eb8eed to
5d23f6f
Compare
|
|
ctavan
force-pushed
the
use-v4-instead-of-v1-uuids
branch
from
9748646 to
93efb58
Compare
| @@ -10,7 +10,7 @@ import shell = require('shelljs'); | |||
| import uuid = require('node-uuid'); | |||
| var ps = shell.which('powershell.exe'); | |||
There was a problem hiding this comment.
undo these. VsTestV1 we are no longer making updates.
|
There's like an infinitesimally small chance of collisions but that shouldn't matter here. This should be fine. Please do test out all the tasks once to ensure everything is working fine. Our E2E tests should also be doing this but i would have one manually pass if possible. Thanks for taking time to contribute. |
ctavan
force-pushed
the
use-v4-instead-of-v1-uuids
branch
from
7817b3c to
25b054f
Compare
This uses a purely random v4 UUID instead of a time-based v1 UUID for all temporary files in the tests. v1 UUID are based on current time and the hardware MAC address of the machine where they are being generated (although the implementation in the npm uuid module uses generates a random fake MAC address). As such they have much more complex semantics than v4 UUIDs which are simply randomly generated. Unless there's a specific requirement for the special semantics of v1 UUIDs it is simpler and less error prone to simply go for v4 UUIDs whenever just a unique identifier is needed.
This uses a purely random v4 UUID instead of a time-based v1 UUID for the feedId in the nuget test (which was introduced in c265d2a). v1 UUID are based on current time and the hardware MAC address of the machine where they are being generated (although the implementation in the npm uuid module uses generates a random fake MAC address). As such they have much more complex semantics than v4 UUIDs which are simply randomly generated. Unless there's a specific requirement for the special semantics of v1 UUIDs it is simpler and less error prone to simply go for v4 UUIDs whenever just a unique identifier is needed.
This uses a purely random v4 UUID instead of a time-based v1 UUID for the taskInstanceIdentifier in the tests (which was introduced in c42775d). v1 UUID are based on current time and the hardware MAC address of the machine where they are being generated (although the implementation in the npm uuid module uses generates a random fake MAC address). As such they have much more complex semantics than v4 UUIDs which are simply randomly generated. Unless there's a specific requirement for the special semantics of v1 UUIDs it is simpler and less error prone to simply go for v4 UUIDs whenever just a unique identifier is needed.
According to https://github.com/kelektiv/node-uuid#uuid- the use of require('uuid') is deprecated and will not be supported after version 3.x of the uuid module. Instead, use require('uuid/[v1|v3|v4|v5]').
ctavan
force-pushed
the
use-v4-instead-of-v1-uuids
branch
from
25b054f to
886ae75
Compare
|
@ShreyasRmsft thanks for taking the time to review my patchset. I have reverted the change to the legacy test.
I don't think that the changes really increase the chance of collisions. In fact collisions with
To be honest I have not the slightest idea how to test this manually as I have never worked with Azure Pipelines (and I do not even have access to an Azure account at this moment). I was hoping that the test suite would provide enough safety to ensure that my changes don't introduce any regressions. Would there be any chance that one of the maintainers of this repo could run the updated code in their environments? As I've stated in my report I'm an author of the |
|
@ctavan no worries, I'll test these out and then complete the pull request. |
|
This PR is stale because it has been open for a year with no activity. Remove the stale label or comment on the PR otherwise this will be closed in 5 days |
3 participants
I'm a co-author of the uuid npm module which is being used in some places within the code base of this repository.
I'm currently trying to understand real-world use cases of time-based UUIDs ("v1 UUIDs").
I found several occurrences in the test suite where
v1UUIDs were being used instead ofv4UUIDs and I was wondering if any of these cases really require the semantically much more complexv1UUIDs or whether we could live equally well with purely randomv4UUIDs? Please see my commit messages for more details.I'd be really curious to understand the motivation for choosing
v1overv4UUIDs in the first place, so any feedback on this would be highly appreciated!@acesiddhu @ShreyasRmsft as far as I can tell from the git history it seems like you have introduced some usages of
v1UUIDs so I'd be particularly interested in your feedback.