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

Create gitlab pipeline #534

Merged
merged 45 commits into from
Dec 2, 2021
Merged

Create gitlab pipeline #534

merged 45 commits into from
Dec 2, 2021

Conversation

alvicsam
Copy link
Contributor

Added gitlab pipeline for this repo.

Closes https://github.com/paritytech/ci_cd/issues/240

.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Thanks alot.

Is the intention to replace all existing Github Actions or just the benches with Gitlab?

I think we need to run the tests too then preferable on Linux, macos and Windows as we do currently.

@alvicsam
Copy link
Contributor Author

Is the intention to replace all existing Github Actions or just the benches with Gitlab?

So initially I was thinking about that because it's easier for CI team to maintain pipelines in one place. But since you have tests on different platforms we have to keep some of the GHA tests for some time

@niklasad1
Copy link
Member

So initially I was thinking about that because it's easier for CI team to maintain pipelines in one place. But since you have tests on different platforms we have to keep some of the GHA tests for some time

Ok, then we should probably remove Check and Check style and Benchmarks GHA then....

@alvicsam alvicsam changed the title As add gitlabci WIP: Create gitlab pipeline Oct 19, 2021
@alvicsam alvicsam changed the title WIP: Create gitlab pipeline Create gitlab pipeline Oct 21, 2021
scripts/ci/pre_cache.sh Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
stage: lint
<<: *kubernetes-env
script:
- rustup component add rustfmt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't the rustfmt component part of the base image? Seems like setting up the toolchain components should be in a single place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually, we run cargo fmt from the nightly release in other repos. I'll add comment here to fix it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I guess the question still stands though: wherever you install the nightly toolchain could be a good place to add the rustfmt component as well? And then run cargo +nightly fmt here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dvdplm We have nightly fmt installed.
I guess you mean that you're OK with cargo +nightly fmt running here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer cargo fmt from stable, but cargo +nightly fmt works too.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a big deal, we can add it to the CI image for stable, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added clippy and fmt to the image however need to wait while issue in substrate is fixed

stage: lint
<<: *docker-env
script:
- rustup component add clippy
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dvdplm We have nightly clippy installed.
I guess you mean that you're OK with cargo +nightly clippy --all-targets running here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I prefer CI to use stable but yes, I'm ok running nightly clippy too. Curious though, why is clippy not included in the base image?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean paritytech/base-ci-linux? It was never requested before to run a stable clippy or fmt.

.gitlab-ci.yml Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
@alvicsam alvicsam requested a review from niklasad1 October 22, 2021 15:15
.gitlab-ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM,

I'm not convinced about moving clippy to Gitlab though because the Github UI is so nice instead of reading text output from Gitlab.

@niklasad1 niklasad1 requested a review from dvdplm October 22, 2021 16:37
@dvdplm
Copy link
Contributor

dvdplm commented Oct 22, 2021

I'm not convinced about moving clippy to Gitlab though because the Github UI is so nice instead of reading text output from Gitlab.

I agree with this.

@TriplEight
Copy link
Contributor

TriplEight commented Nov 4, 2021

@dvdplm @niklasad1 can you please point me to what's wrong with GitLab's clippy output
image

vs a GitHub nice UI's one?
image

Just the highlighted warnings?

Also note that these are the outputs on the same commit, completely different! Both installed on stable Rust.
Regarding if It's OK to keep clippy in GHA: it then runs on a different version of Rust than the one used by our tested CI image against substrate GitLab. And who knows what do you test in this case.
https://gitlab.parity.io/parity/jsonrpsee/-/jobs/1205035
https://github.com/paritytech/jsonrpsee/runs/4109105358?check_suite_focus=true

@TriplEight
Copy link
Contributor

The fact that clippy in GitLab runs 51 sec against 5 sec in GitHub action is due to no cache is used in GitLab for it so far.

@niklasad1
Copy link
Member

@dvdplm @niklasad1 can you please point me to what's wrong with GitLab's clippy output

Yes, it's very unlikely that you would click on the individual jobs when reviewing PRs to look at potential lint warnings.
With Github Actions you get warnings inline in the code review which I really like

Such as
image

Also note that these are the outputs on the same commit, completely different! Both installed on stable Rust.

It's just because the current clippy lint runs cargo clippy and added Gitlab runs cargo clippy --all-targets (including test code and benches)

@alvicsam
Copy link
Contributor Author

alvicsam commented Nov 8, 2021

@niklasad1 I'm a little bit confused. Was this pr closed accidentally or on purpose?

@niklasad1 niklasad1 reopened this Nov 8, 2021
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
stage: lint
<<: *kubernetes-env
script:
- rustup component add rustfmt
Copy link
Contributor

Choose a reason for hiding this comment

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

@dvdplm We have nightly fmt installed.
I guess you mean that you're OK with cargo +nightly fmt running here, right?

stage: lint
<<: *docker-env
script:
- rustup component add clippy
Copy link
Contributor

Choose a reason for hiding this comment

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

@dvdplm We have nightly clippy installed.
I guess you mean that you're OK with cargo +nightly clippy --all-targets running here, right?

- echo ${CI_RUNNER_DESCRIPTION}
- echo "RUNNER_NAME=${CI_RUNNER_DESCRIPTION}" > runner.env

publish-bench:
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to use paritytech/ci-linux here? It's better off with a lighter paritytech/tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better off with a lighter paritytech/tools

Agree, fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

then it maybe makes sense to remove that default image from k8s anchor.

Copy link
Contributor Author

@alvicsam alvicsam Dec 2, 2021

Choose a reason for hiding this comment

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

I'd prefer to keep k8s anchor as it is so it is equal through all our .gitlab-ci.yml files in different repos

.scripts/ci/push_bench_results.sh Outdated Show resolved Hide resolved
alvicsam and others added 2 commits November 30, 2021 15:07
Co-authored-by: Denis Pisarev <17856421+TriplEight@users.noreply.github.com>
stage: lint
<<: *kubernetes-env
script:
- rustup component add rustfmt
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer cargo fmt from stable, but cargo +nightly fmt works too.

stage: lint
<<: *docker-env
script:
- rustup component add clippy
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I prefer CI to use stable but yes, I'm ok running nightly clippy too. Curious though, why is clippy not included in the base image?

scripts/ci/pre_cache.sh Outdated Show resolved Hide resolved
@dvdplm
Copy link
Contributor

dvdplm commented Nov 30, 2021

anyone knows why?

@radupopa2010 I think we need to merge master in here.

@alvicsam
Copy link
Contributor Author

anyone knows why?

rebase fixed the pipeline

@alvicsam
Copy link
Contributor Author

@niklasad1

A follow-up question, is it possible to have similar alerts as the GHA action when regressions are detected?

I think it's possible. I see a few options: send an email, send a matrix message or create a GitHub issue. I can create a new issue in our tracker where we can discuss the way how alert is sent and the conditions.

@alvicsam alvicsam requested a review from TriplEight November 30, 2021 16:43
.gitlab-ci.yml Show resolved Hide resolved
stage: lint
<<: *kubernetes-env
script:
- rustup component add rustfmt
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a big deal, we can add it to the CI image for stable, too.

stage: lint
<<: *docker-env
script:
- rustup component add clippy
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean paritytech/base-ci-linux? It was never requested before to run a stable clippy or fmt.

- echo ${CI_RUNNER_DESCRIPTION}
- echo "RUNNER_NAME=${CI_RUNNER_DESCRIPTION}" > runner.env

publish-bench:
Copy link
Contributor

Choose a reason for hiding this comment

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

then it maybe makes sense to remove that default image from k8s anchor.

@alvicsam alvicsam requested a review from TriplEight December 2, 2021 15:25
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.

8 participants