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

HOTFIX Connect all checks to CI flow #260

Merged
merged 13 commits into from Jun 13, 2018

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Jun 5, 2018

No description provided.

@Waldz Waldz requested review from donce and tadovas as code owners Jun 5, 2018

@Waldz Waldz requested review from interro and zolia and removed request for donce Jun 5, 2018

@ignasbernotas
Copy link
Member

left a comment

LGTM

@Waldz Waldz changed the title HOTFIX Connect all checks CI flow HOTFIX Connect all checks to CI flow Jun 6, 2018

@Waldz Waldz force-pushed the hotfix/travis-checks-linter branch 2 times, most recently from 1ac4b16 to 46e4464 Jun 6, 2018

@@ -129,16 +129,22 @@ this PR will be merged into main branch.

Before creating PR be sure to:

* **Step 1.** Make sure that no linter errors remain in **your** code
* **Step 1.** Ensure sure that **your** code quality is passing

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

"Ensure sure"?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

.travis.yml Outdated
@@ -34,9 +34,11 @@ install:
- glide "-home" $GLIDE_HOME install

script:
- bin/check_vet
- bin/builder_run bin/check_lint

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

Why do we need to run lint in builder?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

I found that Glide dont support for DEV dependencies for developer environment.
So decision for DEV dependencies now is:

  1. all tools documented&frozen in bin/builder_docker/Dockerfile
  2. developer can still install them in local machine

@donce @zolia @tadovas

```

* **Step 2.** Ensure that all unit tests passes, no vet errors remain and code formatted.
* **Step 2.** Ensure that all unit tests passes

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

passes -> pass, because it's plural.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

bin/test
```

* **Step 3.** Ensure that all end-to-end tests passes

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

passes -> pass

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

bin/check Outdated
@@ -0,0 +1,11 @@
#!/bin/bash

# Runs tests and code quality checks

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

Runs tests - it doesn't do that.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

@@ -3,7 +3,7 @@
# Checks if all project files are formatted using go fmt
#
# Usage:
#> bin/check_go_fmt
#> bin/fmt

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

Wrong filename.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

# Run code Linter for project packages
#
# Usage:
#> bin/lint [packages ...]

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

wrong name, same in examples

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Fixed

| grep -v "github.com/mysterium/node/nat" \
| grep -v "github.com/mysterium/node/money" \
| grep -v "github.com/mysterium/node/ip" \
| grep -v "github.com/mysterium/node/location"

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

What does this list mean? Can we add a comment description, or put this list in a variable with clear name?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

Decision is:

  1. We care about quality and run CI checks for new code
  2. We declare packages which are not-clared and clear them in long-run
    @donce @zolia @tadovas

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 8, 2018

Member

Awesome idea. But I would like to have this clear in separate variable like "packagesNeedToClean" or something. Because now I need to look very closely if they are blacklist or whitelist.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 13, 2018

Author Member

Improved

@@ -88,7 +88,5 @@ imports:
subpackages:
- collections/prque
testImports:
- name: github.com/golang/lint

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

Why are we removing lint dependency?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

explained before

import:
- package: github.com/cihub/seelog
version: ^2.6.0

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

It's common practice to allow build version change, i.e. 2.6.1 version should not change behaviour, but will probably contain some fixes.

This comment has been minimized.

Copy link
@donce

donce Jun 7, 2018

Contributor

Also, that's why we have glide.lock to freeze build versions, but changing glide.yaml every time we want to get different build is an overkill and will likely lead to missing important fixes.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 7, 2018

Author Member

My intention was to upgrade 1 package, but Glide does not have this feature. Just glide update
So decision is:

  1. Developer will not randomly brake packages then: adding/removing/updating 1 package (was happening too frequently)
  2. Then we want to update all packages, we: update glide.yaml -> runs glide update

@zolia @tadovas @donce

This comment has been minimized.

Copy link
@donce

donce Jun 8, 2018

Contributor

^2.6.0 means that all 2.6.x versions are allowed - 2.7.0 is not allowed, you would have to edit glide.yaml manually to update to 2.7.0.

was happening too frequently

How frequent was it? Could you give some examples?
It seems unlikely that build change would brake something.. Missing important update, on the other hand, seems more risky.

@Waldz Waldz force-pushed the hotfix/travis-checks-linter branch 2 times, most recently from 8a8d0bd to e3e1829 Jun 7, 2018

@Waldz Waldz force-pushed the feature/MYS-28-cross-platform branch 3 times, most recently from 1cba0e0 to f16cfb8 Jun 8, 2018

@Waldz Waldz force-pushed the hotfix/travis-checks-linter branch from cf75c70 to a658ca4 Jun 8, 2018

@Waldz Waldz changed the base branch from feature/MYS-28-cross-platform to master Jun 8, 2018

@Waldz Waldz dismissed stale reviews from donce and ignasbernotas via 7e40680 Jun 12, 2018

@Waldz Waldz force-pushed the hotfix/travis-checks-linter branch from 8e29865 to 7e40680 Jun 12, 2018

@Waldz Waldz changed the base branch from master to feature/MYST-578-xgo-in-ci Jun 12, 2018

Waldz added some commits Jun 5, 2018

Connect licence check to CI flow
Signed-off-by: Waldz <valdas@mysterium.network>

Waldz added some commits Jun 6, 2018

Improve documentation
Signed-off-by: Waldz <valdas@mysterium.network>
Dont eat linter messages in uncleaned packages
Signed-off-by: Waldz <valdas@mysterium.network>
Freeze how many messages allowed in uncleaned packages
Signed-off-by: Waldz <valdas@mysterium.network>

@Waldz Waldz force-pushed the hotfix/travis-checks-linter branch from 7dbf96f to 91afaf2 Jun 13, 2018

@Waldz Waldz changed the base branch from feature/MYST-578-xgo-in-ci to master Jun 13, 2018

@donce

donce approved these changes Jun 13, 2018


# Checking for license

all_files=`find . -path ./vendor -prune -o -name '*.go' -print`

bad_files=""
for file in $all_files; do
if ! grep -q "$COPYRIGHT" $file

This comment has been minimized.

Copy link
@donce

donce Jun 13, 2018

Contributor

Could you fix the same issue in Mysterion?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 13, 2018

Author Member

Noted a ticket

@Waldz Waldz merged commit 14a9e03 into master Jun 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the hotfix/travis-checks-linter branch Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.