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

MF-1830 - Switch to Semaphore 2.0 #1829

Merged
merged 15 commits into from
Jul 26, 2023
Merged

Conversation

WashingtonKK
Copy link
Contributor

@WashingtonKK WashingtonKK commented Jun 25, 2023

What does this do?

Implement CI using Semaphore 2

Which issue(s) does this PR fix/relate to?

Resolves #1830.

List any changes that modify/break current functionality

  • CI runs tests only on modified modules

Have you included tests for your changes?

N/A

Did you document any new/modified functionality?

Yes

Notes

N/A

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #1829 (cbb9b10) into master (106b710) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1829   +/-   ##
=======================================
  Coverage   64.73%   64.73%           
=======================================
  Files         118      118           
  Lines        9702     9702           
=======================================
  Hits         6281     6281           
  Misses       2753     2753           
  Partials      668      668           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dborovcanin dborovcanin changed the title MF-251 - Change CI MF-1830 - Switch to Semqphore 2.0 Jun 26, 2023
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Comment on lines 13 to 1142
- name: Setup
task:
env_vars:
- name: GO_VERSION
value: 1.16.7
jobs:
- name: Variables
commands:
- NPROC=$(nproc)
- GO_VERSION=1.19.4
- PROTOC_VERSION=21.12
- PROTOC_GEN_VERSION=v1.28.1
- PROTOC_GRPC_VERSION=v1.2.0
- name: Update Go
commands:
- |
function version_gt() {
test "$(printf ''%s\n'' "$@" | sort -V | head -n 1)" != "$1"
}
- |
CURRENT_GO_VERSION=$(go version | sed 's/[^0-9.]*\([0-9.]*\).*/\1/')
- |
if version_gt $GO_VERSION $CURRENT_GO_VERSION; then
echo "Updating go version from $CURRENT_GO_VERSION to $GO_VERSION ..."
sudo rm -rf /usr/bin/go
sudo rm -rf /usr/local/go
sudo rm -rf /usr/local/bin/go
sudo rm -rf /usr/local/golang
sudo rm -rf $GOROOT $GOPATH $GOBIN
wget https://go.dev/dl/go$GO_VERSION.linux-amd64.tar.gz
sudo tar -C /usr/local -xzf go$GO_VERSION.linux-amd64.tar.gz
export GOROOT=/usr/local/go
export PATH=$PATH:/usr/local/go/bin
fi
- export GOBIN=$HOME/go/bin
- 'export PATH=$PATH:$GOBIN'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably do not need to update Go anymore. This was only used as a way of enabling the latest version of Go, but backward compatibility guarantees that it should be just fine if the CI Go version is 1 or 2 (but not more) older than the version in go.mod.

Comment on lines 140 to 157
- checkout
- echo "Running modified tests...."
- echo "" > coverage.txt
- |
# Get the list of modified Go files
MODIFIED_FILES=$(git diff --name-only --diff-filter=AM | grep '\.go$')

# Extract the unique Go module names from modified files
MODIFIED_MODULES=$(go list -e -f '{{.Module.Path}}' $(dirname $MODIFIED_FILES) | sort -u)

# Run tests for modified modules
for module in $MODIFIED_MODULES; do
go test -mod=vendor -v -race -tags test -coverprofile=profile.out -covermode=atomic "$module/..."
if [ -f profile.out ]; then
cat profile.out >> coverage.txt
rm profile.out
fi
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have only one module. Also, detecting changes for testing is more complex than checking changed files, since it depends on which files are changed: if we change access control, for example, we need to test messaging, as well.

@dborovcanin dborovcanin changed the title MF-1830 - Switch to Semqphore 2.0 MF-1830 - Switch to Semaphore 2.0 Jun 26, 2023
@WashingtonKK WashingtonKK force-pushed the setup-semaphore branch 2 times, most recently from 1684eec to f77cf5e Compare June 27, 2023 09:48
Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

Add test for building binaries

- go test -v -coverprofile=coverage.out ./...
- go tool cover -html=coverage.out -o coverage.html
name: Test Bootstrap
- name: Test certs
Copy link
Member

Choose a reason for hiding this comment

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

add tests for cli, coap, consumers, whole http sub package not only /api, and others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it

Copy link
Contributor

Choose a reason for hiding this comment

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

Stay consistent and use capital letter for the name of each service (Certs, Things, ...)

- go test -v -coverprofile=coverage.out ./...
- go tool cover -html=coverage.out -o coverage.html
name: Logger test
- name: Test all
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be correct. I'd suggest to test everything probably during merge to main or after a reviewer has commented LGTM

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is probably no need for this test at all, even in cases we merge to master. However, we need to be sure we tun tests when it is relevant (e.g. when proto files change, we need to run almost all the tests).

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

changeset:
comment:
matches: "LGTM"
when: "branch" = "master" OR "comment" = "LGTM"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

Capitalize service names

@@ -0,0 +1,246 @@
version: v1.0
name: CI Pipeline
Copy link
Member

Choose a reason for hiding this comment

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

The previous CI run took 23.20m yet the current CI runs on the master take around 24m. Shouldn't we be getting faster times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only 3 processes are being run at any instance on the free tier which slows down the whole process. If we had all parallel processes running at the same time concurrently we would have faster times of ~19 min.

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

remove changes not required in this PR

@@ -126,6 +126,7 @@ func main() {
if err := env.Parse(&httpServerConfig, env.Options{Prefix: envPrefixHttp, AltPrefix: envPrefix}); err != nil {
logger.Fatal(fmt.Sprintf("failed to load %s HTTP server configuration : %s", svcName, err))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this change is unnecessary

docker/.env Outdated
@@ -61,6 +61,7 @@ MF_USERS_RESET_PWD_TEMPLATE=users.tmpl
MF_USERS_PASS_REGEX=^.{8,}$$
MF_USERS_INSTANCE_ID=


Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -6,6 +6,7 @@ package api
import (
"context"
"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.

here too

Copy link
Contributor

Choose a reason for hiding this comment

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

probably fixed by rebasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change was associated with a merge from master into the branch to keep it up to date.

@@ -0,0 +1,430 @@
version: v1.0
Copy link
Member

Choose a reason for hiding this comment

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

Remove previous CI

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Please add checks for proto file changes, and also connect changes in Things auth with protocol adapters (i.e. adapters must be tested when auth is changed, but artifacts do not need to be published since the code actually did not change). Since all Lint and Test XYZ have the same pattern, check if we can use some kind of matrix and template hee.

- go test -v -coverprofile=coverage.out ./...
- go tool cover -html=coverage.out -o coverage.html
name: Logger test
- name: Test all
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is probably no need for this test at all, even in cases we merge to master. However, we need to be sure we tun tests when it is relevant (e.g. when proto files change, we need to run almost all the tests).

- docker push mainflux.ws
name: Push Ws

- name: Lint and Test CoAp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix typo: CoAP. Check for other typos.

task:
jobs:
- commands:
- make things
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to build a Docker image, not a binary.

jobs:
- name: Compile For RabbitMQ
commands:
- MF_BROKER_TYPE=rabbitmq make mqtt
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line

Copy link
Contributor

Choose a reason for hiding this comment

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

@WashingtonKK this is still not addressed

}
- setup_protoc
- |
check_files() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

- name: Check Generated Protocol Buffer Files
commands:
- |
setup_protoc() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use functions and call them - you can simply exec this line-by-line.

- name: Things Auth Changes
dependencies: []
run:
when: "change_in('things/policies/auth.proto')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

When changes are in Protobuf files, we need all the tests (i.e. add things/policies/auth.proto to a change_in list for all adapters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of when: "change_in('things/policies/auth.proto')" we need to check for changes in implementation logic of things - something like when: "change_in(['things/policies/postgres', 'things/policies/service.go'])". Also, no need to run coverage in this block since we did not change the actual code of adapters, so no coverage changes will be present.

Comment on lines 771 to 773
commands:
- cd things
- go test ./... --race
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this, it will be triggered by Thing changes.

Comment on lines 853 to 862
- name: Push Docker Images
dependencies:
- Setup
run:
when: branch = 'master'
task:
jobs:
- name: Push Docker Images
commands:
- make -j$NPROC latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this, either. Move publishing to per-service in Build blocks.

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
.semaphore/semaphore.yml Outdated Show resolved Hide resolved
- name: DOCKER_USERNAME
- name: Lint and Test Things
run:
when: "change_in(['things', 'cmd/things', 'users/policies/auth.proto'])"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's test if changes are in /users/policies/postgres/policies.go and /users/policies/api/grpc/client.go, not user/policies/auth.proto.

.semaphore/semaphore.yml Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

@WashingtonKK You are missing these:

- echo $DOCKER_TOKEN | docker login --username "$DOCKER_USERNAME" --password-stdin
- docker push mainflux/http:latest

Ofc, this is for HTTP and we need for others as well.

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 2b78902 into absmach:master Jul 26, 2023
3 checks passed
WashingtonKK added a commit to WashingtonKK/magistrala that referenced this pull request Jul 28, 2023
* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add Docker secret

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify block code

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove coverage on test block

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fixes

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Update badge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove repeated tests'

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify docker secrets

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Push docker images

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

---------

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Co-authored-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
WashingtonKK added a commit to WashingtonKK/magistrala that referenced this pull request Jul 28, 2023
* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add Docker secret

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify block code

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove coverage on test block

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fixes

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Update badge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove repeated tests'

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify docker secrets

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Push docker images

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

---------

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Co-authored-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
WashingtonKK added a commit to WashingtonKK/magistrala that referenced this pull request Jul 28, 2023
* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add Docker secret

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify block code

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove coverage on test block

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fixes

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Update badge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove repeated tests'

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify docker secrets

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Push docker images

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

---------

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Co-authored-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
rodneyosodo pushed a commit to rodneyosodo/magistrala that referenced this pull request Aug 3, 2023
* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add pipeline

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Add Docker secret

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove function

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify block code

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove coverage on test block

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fixes

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Update badge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Remove repeated tests'

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Fix typo

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Modify docker secrets

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

* Push docker images

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

---------

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Co-authored-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
@WashingtonKK WashingtonKK deleted the setup-semaphore branch March 12, 2024 07:37
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.

Switch to Semaphore 2.0
7 participants