Skip to content
This repository has been archived by the owner on Feb 21, 2024. It is now read-only.

Upgrade to v2 #2072

Merged
merged 1 commit into from Oct 9, 2019
Merged

Upgrade to v2 #2072

merged 1 commit into from Oct 9, 2019

Conversation

benbjohnson
Copy link
Contributor

Overview

Major version upgrade of go.mod to v2. I will tag this commit as v2.0.0 once it is merged to master.

The previous v1.x.x line is now on the v1 branch.

Pull request checklist

  • I have read the contributing guide.
  • I have agreed to the Contributor License Agreement.
  • I have resolved any merge conflicts.
  • I have included tests that cover my changes.
  • All new and existing tests pass.
  • Make sure PR title conforms to convention in CHANGELOG.md.
  • Add appropriate changelog label to PR (if applicable).

Code review checklist

This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

  • Ensure that any changes to external docs have been included in this pull request.
  • If the changes require that minor/major versions need to be updated, tag the PR appropriately.
  • Ensure the new code is properly commented and follows Idiomatic Go.
  • Check that tests have been written and that they cover the new functionality.
  • Run tests and ensure they pass.
  • Build and run the code, performing any applicable integration testing.
  • Make sure PR title conforms to convention in CHANGELOG.md.
  • Make sure PR is tagged with appropriate changelog label.

@jaffee
Copy link
Member

jaffee commented Oct 8, 2019

Looks good to me, but I'd appreciate it if @codysoyland could give the final OK

@jaffee
Copy link
Member

jaffee commented Oct 8, 2019

OK, I nerd-sniped myself trying to check my assumptions about which files need to change - I'm wondering about a few files that aren't changed.

  • Makefile - specifically the LDFLAGS and go generate stuff.
  • roaring/roaring_stats.go
  • server/enterprise.go
  • server/setup_logger_arm64.go

I think these files should be changed, and if I'm right, we should understand out why our CI checks didn't catch them.

@codysoyland
Copy link
Contributor

codysoyland commented Oct 8, 2019

The places @jaffee mentioned all do need to be fixed. This commit addresses everything I could find: codysoyland@995e18f

This does show a few deficiencies in CI. CI does not currently exercise go generate and there is no test that verifies that LDFLAGS is set. server/enterprise.go and roaring/roaring_stats.go are protected by build tags so not included by default. server/setup_logger_arm64.go only builds on arm, but it and enterprise were successfully able to build due to (I think) go module caching in CircleCI.

It might be worth updating CONTRIBUTING.md to a module-first environment, but that would require some unclear conventions.

Also, after this commit is merged, I think we might want to tag it something like v2.0.0-alpha.1 as there are some other things we might want to address in git grep -i "todo" | grep 2\.0 as well as updating the CHANGELOG and docs (per the release instructions) before cutting the actual v2.0.0.

Co-authored-by: Cody Soyland <codysoyland@gmail.com>
@benbjohnson
Copy link
Contributor Author

It might be worth updating CONTRIBUTING.md to a module-first environment, but that would require some unclear conventions.

@codysoyland I wasn't quite sure what you meant by this. I read through and I didn't see parts that need to change to support v2.

@benbjohnson
Copy link
Contributor Author

Also, I cherry-picked your changes and pushed up a rebase, coauthored commit.

@codysoyland
Copy link
Contributor

It might be worth updating CONTRIBUTING.md to a module-first environment, but that would require some unclear conventions.

@codysoyland I wasn't quite sure what you meant by this. I read through and I didn't see parts that need to change to support v2.

I was thinking about removing references to GOPATH, but on second thought, it's really not relevant here. It just came up when I was grepping for the import path.

Copy link
Member

@jaffee jaffee left a comment

Choose a reason for hiding this comment

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

🆗

@benbjohnson benbjohnson merged commit f9e4787 into FeatureBaseDB:master Oct 9, 2019
@benbjohnson benbjohnson deleted the upgrade-v2 branch October 9, 2019 14:34
@benbjohnson
Copy link
Contributor Author

f9e478722cbc8880718439614b5bf97806e70403 is now tagged as v2.0.0-alpha.1 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants