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: Initial new pipeline #3505

Merged
merged 12 commits into from
Dec 11, 2019
Merged

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Dec 10, 2019

This change is Reviewable

@lukedirtwalker lukedirtwalker force-pushed the pubCI2 branch 5 times, most recently from 630148e to 18b9127 Compare December 10, 2019 13:17
@lukedirtwalker lukedirtwalker changed the title WIP: CI: Initial new pipeline CI: Initial new pipeline Dec 10, 2019
@lukedirtwalker lukedirtwalker marked this pull request as ready for review December 10, 2019 13:23
Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @worxli)


.bazelrc_ci, line 1 at r1 (raw file):

build --workspace_status_command=./tools/bazel-build-env --remote_cache=http://localhost:8080 --stamp

is stamp needed/useful?


.buildkite/pipeline.yml, line 9 at r1 (raw file):

        - exit_status: -1  # Agent was lost
        - exit_status: 255 # Forced agent shutdown
    timeout_in_minutes: 10

is 10 still accurate?


.buildkite/pipeline.yml, line 19 at r1 (raw file):

        - exit_status: -1  # Agent was lost
        - exit_status: 255 # Forced agent shutdown
    timeout_in_minutes: 10

here too? do we need a timeout at all?


.buildkite/hooks/pre-command, line 4 at r1 (raw file):


set -e

add an echo that says what is done


.buildkite/hooks/pre-command, line 6 at r1 (raw file):


# Start bazel remote cache proxy for S3
docker run -e  BAZEL_REMOTE_S3_BUCKET=buildkite-bazel-cache\

remove space after -e


.buildkite/hooks/pre-command, line 8 at r1 (raw file):

docker run -e  BAZEL_REMOTE_S3_BUCKET=buildkite-bazel-cache\
           -e BAZEL_REMOTE_S3_ENDPOINT=s3.eu-central-1.amazonaws.com\
           -e BAZEL_REMOTE_S3_ACCESS_KEY_ID=$BAZEL_REMOTE_S3_ACCESS_KEY_ID\

where are the env vars coming from now?

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @worxli)


.buildkite/pipeline.yml, line 9 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

is 10 still accurate?

It's probably way too high, but I'd rather have some conservative value than nothing.


.buildkite/pipeline.yml, line 19 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

here too? do we need a timeout at all?

It's probably way too high, but I'd rather have some conservative value than nothing.


.buildkite/hooks/pre-command, line 4 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

add an echo that says what is done

Done.


.buildkite/hooks/pre-command, line 6 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

remove space after -e

Done.


.buildkite/hooks/pre-command, line 8 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

where are the env vars coming from now?

I added an env script in the S3 bucket where the github access key is. From the values are exported. (see https://buildkite.com/docs/pipelines/secrets#storing-secrets-with-the-elastic-ci-stack-for-aws)

Also added a comment above.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @worxli)


.bazelrc_ci, line 1 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

is stamp needed/useful?

don't know we always had this? no?

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


.buildkite/pipeline.yml, line 14 at r1 (raw file):

      - ./scion.sh test go
    artifact_paths:
      - "bazel-testlogs/**/*"

needs fixing

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)


.buildkite/pipeline.yml, line 12 at r2 (raw file):

  - label: ":bazel: Go tests"
    command:
      - ./scion.sh test go

would it be possible to run this without wrapper script?
bazel test //go/... --print_relative_test_log_paths --nocache_test_results --runs_per_test=10


.buildkite/hooks/pre-command, line 3 at r2 (raw file):

#!/bin/bash

set -e

Consider using set -euo pipefail to safer if a variable is no initialized


.buildkite/hooks/pre-command, line 16 at r2 (raw file):

           --net=host -d buchgr/bazel-remote-cache
# To use it replace the bazelrc file
cp .bazelrc_ci .bazelrc

if bazelrc_ci is one liner file just have a hardcode string and echo to the write location.
Also the .bazelrc file might be define with ENV (avoid better do stuff with . unless you make sure where you are placed)

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karampok and @lukedirtwalker)


.buildkite/pipeline.yml, line 12 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

would it be possible to run this without wrapper script?
bazel test //go/... --print_relative_test_log_paths --nocache_test_results --runs_per_test=10

It might just get out of sync real quickly


.buildkite/hooks/pre-command, line 3 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

Consider using set -euo pipefail to safer if a variable is no initialized

👍

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @worxli)


.buildkite/pipeline.yml, line 14 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

needs fixing

done (maybe)


.buildkite/pipeline.yml, line 12 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

It might just get out of sync real quickly

I think I would leave it for now. I don't think we want to do 10 runs per test, by default. This would mean you can never cache. There should be an option to run tests multiple times, but I don't think it should be the default.


.buildkite/hooks/pre-command, line 3 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

👍

Done.


.buildkite/hooks/pre-command, line 16 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

if bazelrc_ci is one liner file just have a hardcode string and echo to the write location.
Also the .bazelrc file might be define with ENV (avoid better do stuff with . unless you make sure where you are placed)

Done.

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)


.buildkite/pipeline.yml, line 12 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think I would leave it for now. I don't think we want to do 10 runs per test, by default. This would mean you can never cache. There should be an option to run tests multiple times, but I don't think it should be the default.

yeah that it okay, it was just copy-paste example how I run bazel locally.


.buildkite/hooks/pre-artifact, line 11 at r4 (raw file):

if [ "$BUILDKITE_PULL_REQUEST" == "false" ]; then
    TARGET="$BUILDKITE_BRANCH"
else

do it in golang way

TARGET=X
if something then
TARGET =Y
fi

.buildkite/hooks/pre-artifact, line 18 at r4 (raw file):

[ -n "$NIGHTLY" ] && BUILD=nightly-"$(date +%s)"

ARTIFACTS="buildkite.${BUILDKITE_ORGANIZATION_SLUG}.${TARGET}.${BUILD}.${BUILDKITE_STEP_KEY}"

Consider adding a random number, if I remember correctly running twice a step we lose the output, no?

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @worxli)

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 2 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @worxli)

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @karampok and @worxli)


.buildkite/hooks/pre-artifact, line 11 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

do it in golang way

TARGET=X
if something then
TARGET =Y
fi

Done.


.buildkite/hooks/pre-artifact, line 18 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

Consider adding a random number, if I remember correctly running twice a step we lose the output, no?

Added JOB_ID which is a UUID for each job.

Copy link
Contributor

@karampok karampok left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli)

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r3.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @karampok and @lukedirtwalker)


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

cp -R "bazel-testlogs" "$ARTIFACTS_DIR"
cp -R "logs" "$ARTIFACTS_DIR"
tar chaf "artifacts.out/$ARTIFACTS.tar.gz" artifacts

now you have an artificats folder in the tar?

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @karampok and @lukedirtwalker)


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

cp -R "bazel-testlogs" "$ARTIFACTS_DIR"
cp -R "logs" "$ARTIFACTS_DIR"
tar chaf "artifacts.out/$ARTIFACTS.tar.gz" artifacts

also, this should be tar chaf "artifacts.out/$ARTIFACTS.tar.gz" "$ARTIFACTS_DIR"

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @karampok and @lukedirtwalker)


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

cp -R "bazel-testlogs" "$ARTIFACTS_DIR"
cp -R "logs" "$ARTIFACTS_DIR"
tar chaf "artifacts.out/$ARTIFACTS.tar.gz" artifacts

tar chaf "artifacts.out/$ARTIFACTS.tar.gz" -C "$ARTIFACTS_DIR" .

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @karampok and @worxli)


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

now you have an artificats folder in the tar?

Done.


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

also, this should be tar chaf "artifacts.out/$ARTIFACTS.tar.gz" "$ARTIFACTS_DIR"

Done.


.buildkite/hooks/pre-artifact, line 29 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

tar chaf "artifacts.out/$ARTIFACTS.tar.gz" -C "$ARTIFACTS_DIR" .

Done.

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit c9f420c into scionproto:master Dec 11, 2019
@lukedirtwalker lukedirtwalker deleted the pubCI2 branch December 11, 2019 15:15
@lukedirtwalker lukedirtwalker mentioned this pull request Dec 17, 2019
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