-
Notifications
You must be signed in to change notification settings - Fork 47
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: Migrate Gitlab tests to GitHub runner #698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say I like this structure much more. Few points:
- The get-head-commit can be a simple step and not a full job, as it kinda pollutes the space for the actions that are actually meaningful. And since we can't re-use it anyway, I would suggest to make it a simple step for all jobs that need it, and reference it as a step output instead.
- Already mentioned in a comment, but let's aim to have checks, tests and anything that does not result in a build on GH, and everything else on GL. These include Docker images, WASM blobs, and perhaps even the docs, if possible, since we can check them on GH and then deploy them from GL. If deploying the docs from GL is too complicated, I am willing to accept that we deploy them from GH, althought I wouldn't be super happy about it.
- Not sure we need the Dockerx stuff in GH anymore, since we can specify a container to run the action within, and environment variables as well as part of the step or job or even workflow definition.
I will take a more thorough review once these are addressed, but as I said in the beginning, this looks already much better compared to what we now have, which is already much much better than what we had last week 🔥
restore-keys: | | ||
$ ${{ github.job }}-${{ github.ref_name }}-clippy-all-features- | ||
|
||
- name: Set up Docker Buildx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? Doesn't GH support the container
instruction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use the container due to resource constraints it bring yet removed this step
.gitlab-ci.yml
Outdated
interruptible: true | ||
|
||
.check_skip_rust: | ||
build-images: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move WASM building and artifacts as well to GL? SO we have all checks and tests on GH, and everything we publish we do via GL, instead of having to jump between the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, only docs publishing will stay on gh
.github/workflows/build-images.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather trigger-build-images-gitlab
or something to make it clear.
.github/workflows/build-images.yml
Outdated
GITLAB_TRIGGER_TOKEN: ${{ secrets.GITLAB_TRIGGER_TOKEN }} | ||
GITLAB_PROJECT_ID: ${{ secrets.GITLAB_PROJECT_ID }} | ||
run: | | ||
curl -X POST \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say I like this structure much more. Few points:
- The get-head-commit can be a simple step and not a full job, as it kinda pollutes the space for the actions that are actually meaningful. And since we can't re-use it anyway, I would suggest to make it a simple step for all jobs that need it, and reference it as a step output instead.
- Already mentioned in a comment, but let's aim to have checks, tests and anything that does not result in a build on GH, and everything else on GL. These include Docker images, WASM blobs, and perhaps even the docs, if possible, since we can check them on GH and then deploy them from GL. If deploying the docs from GL is too complicated, I am willing to accept that we deploy them from GH, althought I wouldn't be super happy about it.
- Not sure we need the Dockerx stuff in GH anymore, since we can specify a container to run the action within, and environment variables as well as part of the step or job or even workflow definition.
I will take a more thorough review once these are addressed, but as I said in the beginning, this looks already much better compared to what we now have, which is already much much better than what we had last week 🔥
fixes KILTProtocol/ticket#3345
Build docker image only when tests on gh passed
Checklist:
array[3]
useget(3)
, ...)