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.

Sign up for GitHub

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

Jump to bottom

UPPSF-1150 Refactored project #35

Merged
merged 13 commits into from
Mar 18, 2020
Merged

UPPSF-1150 Refactored project #35

merged 13 commits into from
Mar 18, 2020

Conversation

powerslider
Copy link
Contributor

  • Integrated go modules
  • Extracted constants to yaml configuration
  • Restructured project with a proper cmd/pkg structure
  • Set proper visibility to all structs and interfaces
  • Fixed tests to comply with the new structure
  • Added new UPPLogger
  • Extracted client and server HTTP boilerplate outside of main.go
  • Added docker-compose.yml and Makefile

UPPSF-1150 Refactored project
f952bbf 
- Integrated go modules
- Extracted constants to yaml configuration
- Restructured project with a proper cmd/pkg structure
- Set proper visibility to all structs and interfaces
- Fixed tests to comply with the new structure
- Added new UPPLogger
- Extracted client and server HTTP boilerplate outside of main.go
- Added docker-compose.yml and Makefile
rnov
rnov suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@rnov rnov left a comment

Choose a reason for hiding this comment

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

Besides the order of imports. Just one more thing regarding the project's structure:

packages like es mapper and/or mapper should/could it be a subpackages of the same one ? (just out of curiosity)

README.md
@@ -75,4 +75,4 @@ Shows ES cluster health details
## Other information
An example of event structure is here [testdata/exampleEnrichedContentModel.json](messaging/testdata/exampleEnrichedContentModel.json)

The reference mappings for Elasticsearch are found here [runtime/referenceSchema.json](runtime/referenceSchema.json)
The reference mappings for Elasticsearch are found here [configs/referenceSchema.json](configs/referenceSchema.json)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing updates: tests no longer are used with vendor (?); missing info regarding the logger;
refer : https://github.com/Financial-Times/upp-schema-reader/blob/master/README.md; https://github.com/Financial-Times/public-lists-api/blob/master/README.md

.circleci/config.yml Outdated Show resolved Hide resolved
cmd/content-rw-elasticsearch/main.go Outdated
@@ -1,36 +1,24 @@
package main

import (
"net"
"net/http"
"github.com/Financial-Times/content-rw-elasticsearch/pkg/concept"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports order; As noted above having a linter that checks the format helps a lot;
https://github.com/Financial-Times/upp-coding-standard

docker-compose.yml Outdated
driver: local

networks:
common:
Copy link
Contributor

Choose a reason for hiding this comment

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

missing eol.

pkg/config/config.go Outdated

import (
"fmt"
"github.com/Financial-Times/content-rw-elasticsearch/pkg/schema"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports order

pkg/es/service.go
package es

import (
"encoding/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports order

pkg/health/healthcheck.go
HTTPClient *http.Client
Checks []fthealth.Check
AppSystemCode string
log *logger.UPPLogger
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason log not to be public ? (just an opinion)

test/helper.go Outdated
log.Fatal(err)
}
return content
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing eol

pkg/message/message_handler.go
package message

import (
"encoding/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports order

@coveralls
Copy link

coveralls commented Feb 20, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 96.644% when pulling cef9967 on feature/UPPSF-1150-refactor-and-upp-logging into cf5e568 on master.

MiroslavGatsanoga
MiroslavGatsanoga reviewed Feb 20, 2020
View reviewed changes
Copy link

@MiroslavGatsanoga MiroslavGatsanoga left a comment

Choose a reason for hiding this comment

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

We should remove the failing Snyk checks and use the golang-ci orb in order do run a Snyk scan.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/health/healthcheck.go Outdated Show resolved Hide resolved
pkg/http/server.go Outdated Show resolved Hide resolved
pkg/mapper/mapper.go Outdated Show resolved Hide resolved
test/helper.go Show resolved Hide resolved
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch 3 times, most recently from bd36972 to 62449e0 Compare February 20, 2020 16:37
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch from 62449e0 to 0dd44eb Compare February 20, 2020 16:38
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch 10 times, most recently from 7e59fb5 to fd01324 Compare March 11, 2020 14:35
UPPSF-1150 Added v2 to go.mod and all imports
bd4437e 
- Removed unneeded mutexes
- Added install target in Makefile for installing statik circleci
- Deleted generated statik binary which will now be generated only
before build
- Renamed test/data to test/testdata to ignore being detected from go
tooling
- Added statik as an external dependency to Dockerfile and circleci
config
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch from fd01324 to bd4437e Compare March 11, 2020 14:45
MiroslavGatsanoga
MiroslavGatsanoga reviewed Mar 11, 2020
View reviewed changes
.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
pkg/message/message_handler.go Outdated Show resolved Hide resolved
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch 2 times, most recently from 424f670 to ecc308c Compare March 12, 2020 15:56
@powerslider powerslider force-pushed the feature/UPPSF-1150-refactor-and-upp-logging branch from ecc308c to c0b18bd Compare March 13, 2020 08:45
@rnov rnov self-requested a review March 17, 2020 12:33
rnov
rnov previously approved these changes Mar 17, 2020
View reviewed changes
MiroslavGatsanoga
MiroslavGatsanoga reviewed Mar 17, 2020
View reviewed changes
pkg/message/message_handler.go Outdated Show resolved Hide resolved
pkg/es/client.go Outdated Show resolved Hide resolved
@powerslider powerslider merged commit 290c236 into master Mar 18, 2020
@powerslider powerslider deleted the feature/UPPSF-1150-refactor-and-upp-logging branch March 18, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnov rnov rnov approved these changes

@MiroslavGatsanoga MiroslavGatsanoga MiroslavGatsanoga left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@powerslider @coveralls @MiroslavGatsanoga @rnov