Skip to content
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

Fix/uppsf 2909 remove neoutils #25

Merged
merged 4 commits into from
Dec 20, 2021
Merged

Conversation

mchompalova
Copy link
Contributor

@mchompalova mchompalova commented Dec 16, 2021

Description

What

We removed the dependency on neo-utils-go from the project.

Why

https://financialtimes.atlassian.net/browse/UPPSF-2909

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

This Pull Request follows the rules described in our Pull Requests Guide

@mchompalova mchompalova requested a review from a team as a code owner December 16, 2021 12:33
Copy link
Contributor

@ManoelMilchev ManoelMilchev left a comment

Choose a reason for hiding this comment

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

We have a little issue in docker-compose-tests.yml, the rest are non-blocking suggestions.

@@ -10,7 +10,7 @@ workflows:
name: build-and-test-project
executor-name:
name: ft-golang-ci/default-with-neo4j
neo4j-image-version: 3.5.28-enterprise
neo4j-image-version: 4.3.6-enterprise
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: We can remove the explicit neo4j-image-version as our circleci executor default-with-neo4j version is the same.

command: [ "go", "test", "-mod=readonly", "-tags=integration" , "./..." ]
depends_on:
- neo4j
neo4j:
image: neo4j:3.5.28-enterprise
image: neo4j:3.4.6-enterprise
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I think that you mistyped the image version. The version that we are looking for is 4.3.6-enterprise.The tests wont execute with the current 3.4.6-enterprise version.

@@ -10,13 +10,12 @@ services:
GITHUB_USERNAME: "${GITHUB_USERNAME}"
GITHUB_TOKEN: "${GITHUB_TOKEN}"
environment:
- NEO4J_TEST_URL=http://neo4j:7474
- NEO4J_BOLT_TEST_URL=bolt://neo4j:7687
- NEO4J_TEST_URL=bolt://neo4j:7687
command: [ "go", "test", "-mod=readonly", "-tags=integration" , "./..." ]
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Can we add the verbose flag please.

Suggested change
command: [ "go", "test", "-mod=readonly", "-tags=integration" , "./..." ]
command: [ "go", "test", "-v", "-mod=readonly", "-tags=integration" , "./..." ]

@coveralls
Copy link

coveralls commented Dec 20, 2021

Coverage Status

Coverage remained the same at 78.947% when pulling 83217e8 on fix/UPPSF-2909-remove-neoutils into 1b36716 on master.

Copy link
Contributor

@ManoelMilchev ManoelMilchev left a comment

Choose a reason for hiding this comment

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

LGTM

@mchompalova mchompalova merged commit 419d114 into master Dec 20, 2021
@mchompalova mchompalova deleted the fix/UPPSF-2909-remove-neoutils branch December 20, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants