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

Change UUID package to github.com/hashicorp/go-uuid #24

Merged
merged 1 commit into from Jun 16, 2018

Conversation

Projects
None yet
3 participants
@kenshaw
Contributor

kenshaw commented Jun 16, 2018

The satori UUID package causes problems with go get and vgo when
building multiple database drivers in the same Go application. This is
because the satori package introduced breaking API changes and has not
tagged it a new version yet.

This change converts the connection ID generation to use the hashicorp
UUID package which was already a dependency of this project, as it is a
dependency of the gokrb package.

This has the added benefit of reducing the number of total package
dependencies by 1.

Additionally, this is needed by github.com/xo/usql so that go get and
vgo build both work "out of the box".

The following is a small Go program that demonstrates that the UUIDs
generated will be the same format:

package main

import (
	"log"

	guuid "github.com/google/uuid"
	huuid "github.com/hashicorp/go-uuid"
	suuid "github.com/satori/go.uuid"
)

func main() {
	g, err := guuid.NewRandom()
	if err != nil {
		log.Fatal(err)
	}

	h, err := huuid.GenerateUUID()
	if err != nil {
		log.Fatal(err)
	}

	s, err := suuid.NewV4()
	if err != nil {
		log.Fatal(err)
	}

	log.Printf(
		"google: %s, hashicorp: %s, satori: %s",
		g, h, s,
	)
}
@F21

This comment has been minimized.

Member

F21 commented Jun 16, 2018

@kenshaw Thanks, this is a good idea! As this project is now part of the Apache Calcite project and the change is not trivial, can you please do the following:

  • Open an issue in JIRA and set the component to avatica-go: https://issues.apache.org/jira/projects/CALCITE/issues
  • Update your commit message to reflect the JIRA issue number and include your name, ex: [CALCITE-1234] Change UUID package to github.com/hashicorp/go-uuid (Ken Shaw)

Thanks! 😄

@kenshaw

This comment has been minimized.

Contributor

kenshaw commented Jun 16, 2018

@F21

This comment has been minimized.

Member

F21 commented Jun 16, 2018

@kenshaw, thanks that looks great! As you're not a committer of Apache Calcite, can you please include your name in parenthesis at the end of the commit message?

@kenshaw

This comment has been minimized.

Contributor

kenshaw commented Jun 16, 2018

@F21 updated.

@F21

This comment has been minimized.

Member

F21 commented Jun 16, 2018

@kenshaw Can you use ( instead of [ when including your name? Sorry for being picky, but these are the standards being set by Apache Calcite.

[CALCITE-2367] Change UUID package (Kenneth Shaw)
The satori UUID package causes problems with go get and vgo when
building multiple database drivers in the same Go application. This is
because the satori package introduced breaking API changes and has not
tagged it a new version yet.

This change converts the connection ID generation to use the hashicorp
UUID package which was already a dependency of this project, as it is a
dependency of the gokrb package.

This has the added benefit of reducing the number of total package
dependencies by 1.

Additionally, this is needed by github.com/xo/usql so that go get and
vgo build both work "out of the box".

The following is a small Go program that demonstrates that the UUIDs
generated will be the same format:

package main

import (
	"log"

	guuid "github.com/google/uuid"
	huuid "github.com/hashicorp/go-uuid"
	suuid "github.com/satori/go.uuid"
)

func main() {
	g, err := guuid.NewRandom()
	if err != nil {
		log.Fatal(err)
	}

	h, err := huuid.GenerateUUID()
	if err != nil {
		log.Fatal(err)
	}

	s, err := suuid.NewV4()
	if err != nil {
		log.Fatal(err)
	}

	log.Printf(
		"google: %s, hashicorp: %s, satori: %s",
		g, h, s,
	)
}

Fixes CALCITE-2367.
@kenshaw

This comment has been minimized.

Contributor

kenshaw commented Jun 16, 2018

@F21 done.

@F21

This comment has been minimized.

Member

F21 commented Jun 16, 2018

@kenshaw Thanks! I'll merge it when the tests turn green. As the project is now part of the Apache Foundation, I will need to start a vote on the mailing list in order to tag a release. How urgent is your dependency on this fix?

@kenshaw

This comment has been minimized.

Contributor

kenshaw commented Jun 16, 2018

I actually don't need this merged any time soon. I've already fixed the dependency issues using vgo.

Also, please note that you might want to consider tagging releases as vX.X.X instead of X.X.X in the future, as only the vX.X.X (I believe) is going to be recognized by the vgo standard.

@F21

This comment has been minimized.

Member

F21 commented Jun 16, 2018

Thanks for the heads up, I'll make sure to include the v in the tag for the next release. As the fix is not super urgent, I'll wait for a few more commits before starting a vote for a release.

@asfgit asfgit merged commit bb93a10 into apache:master Jun 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment