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

ci: added generators for build, test & helm sync #3

Closed
wants to merge 1 commit into from

Conversation

mzedel
Copy link
Contributor

@mzedel mzedel commented Aug 30, 2024

@alfrunes as RFC, resulting in: https://gist.github.com/mzedel/690a2fc1a3bde898ac17b10f2c912b22 - to be included via https://docs.gitlab.com/ee/ci/pipelines/downstream_pipelines.html#dynamic-child-pipelines and potentially extended with the per-service coverage publish jobs...
BUT! I just assume we would get the Dockerfile/ pkg issue to work somehow 🤞

- very WIP, such JS

Ticket: None
Changelog: None
Signed-off-by: Manuel Zedel <[email protected]>
Copy link
Contributor

@alfrunes alfrunes left a comment

Choose a reason for hiding this comment

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

There are some jobs missing (none for frontend, build and run backend acceptance and integration tests), as well as publishing so the trigger jobs would not work.
Another issue we will eventually have to deal with is how to deal with test coverage. By excluding jobs, it will also exclude test coverage unless we have a cleaver way of caching coverage. Or should we drop coverage collection?

Comment on lines +64 to +76
- docker buildx build
--cache-to type=registry,ref=${CI_REGISTRY_IMAGE}:ci_cache,mode=max
--cache-from type=registry,ref=${CI_REGISTRY_IMAGE}:ci_cache
--tag \${GITLAB_REGISTRY_TAG}
--tag ${CI_REGISTRY_IMAGE}:${CI_COMMIT_REF_NAME}
--file backend/services/${service}/Dockerfile
--build-arg GIT_COMMIT_TAG="${DOCKER_PUBLISH_COMMIT_TAG}"
--platform $MULTIPLATFORM_PLATFORMS
--provenance false
--push
\${EXTRA_DOCKER_ARGS}
.
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead use the make targets? That makes it easier to reproduce locally and make it more obvious to compare local vs CI builds.

make -C backend/services/${service} docker

RULES_CHANGES_COMPARE_TO_REF,
service,
}) => `
build:${service}:docker-multiplatform:
Copy link
Contributor

Choose a reason for hiding this comment

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

Building services in different jobs, although parallel is more wasteful as it cannot leverage the local go cache.

Comment on lines +4 to +15
"auditlogs",
"create-artifact-worker",
"deployments",
"deviceauth",
"deviceconfig",
"deviceconnect",
"devicemonitor",
"inventory",
"mender-gateway",
"tenantadm",
"useradm",
"workflows",
Copy link
Contributor

Choose a reason for hiding this comment

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

This list contains enterprise services and mender-gateway which is not part of this repository.

- job: build:${service}:docker-multiplatform
artifacts: true
script:
- go test -trimpath -ldflags -s -w ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we deal with the test with the same toolchain version as we're building. Today we're not doing a very good job at that - versions managed inside gitlab tend to lag major versions (Go term - not semver) behind the Dockerfile.

Suggested change
- go test -trimpath -ldflags -s -w ./...
- go test -trimpath -ldflags='-s -w' ./...

tags:
- hetzner-amd-beefy
services:
- name: ${CI_DEPENDENCY_PROXY_DIRECT_GROUP_IMAGE_PREFIX}/docker:20.10.21-dind
Copy link
Contributor

Choose a reason for hiding this comment

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

This version of docker is over 3 years old. Could we make the docker version more transparent in the global variable section?

Suggested change
- name: ${CI_DEPENDENCY_PROXY_DIRECT_GROUP_IMAGE_PREFIX}/docker:20.10.21-dind
- name: ${CI_DEPENDENCY_PROXY_DIRECT_GROUP_IMAGE_PREFIX}/docker:${DOCKER_VERSION}-dind

Comment on lines +56 to +58
rules:
${getChangesRule({ RULES_CHANGES_COMPARE_TO_REF, service })}
- if: $CI_COMMIT_BRANCH =~ /^(staging)$/
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is a topic for the QA meeting, but IMO we could drop the staging branch and deploy to staging from main.

Suggested change
rules:
${getChangesRule({ RULES_CHANGES_COMPARE_TO_REF, service })}
- if: $CI_COMMIT_BRANCH =~ /^(staging)$/
rules:
${getChangesRule({ "HEAD~1", service })}
- if: $CI_COMMIT_BRANCH == main

@mzedel mzedel mentioned this pull request Sep 5, 2024
@oldgiova
Copy link
Contributor

With regard to the Helm Chart sync, although I consider this a great rework, some key points have to be addressed before:

  1. the idea is to use release tags from main; this mean that the helm:sync job should be extended to run just once, and increment every services in the helm chart (requires rework on the helm chart side)
  2. you create a new release tag for every services, then the deviceconnect service is always restarted on every release, even without changes on that service. This is a concern for long living websocket connections @alfrunes.
  3. The helm sync should run from the mender-server-enterprise repo only ;)

@mzedel mzedel closed this Sep 30, 2024
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.

3 participants