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

Devops ci/move fuzz to reusable workflow #133

Merged
merged 9 commits into from
Sep 5, 2022

Conversation

nathan-bo
Copy link
Contributor

No description provided.

@nathan-bo nathan-bo requested review from isaac-io and maxb-io August 23, 2022 10:40
.github/workflows/test_fuzz.yml Outdated Show resolved Hide resolved
.github/workflows/test_fuzz.yml Outdated Show resolved Hide resolved
.github/workflows/test_fuzz.yml Outdated Show resolved Hide resolved
.github/workflows/test_fuzz.yml Outdated Show resolved Hide resolved
Comment on lines 62 to 63
chmod +x prepfuz.sh
bash -xv prepfuz.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply run the commands instead of outputting them to a file and executing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it was ran in gitlab ci, I think @maxb-io explained it to me but I can't really remember

.github/workflows/test_fuzz.yml Outdated Show resolved Hide resolved
Comment on lines 68 to 70
- name: Copy ${{ matrix.name }} logs to S3
run: |
aws s3 cp $GITHUB_WORKSPACE/out/${{ matrix.name }}.log-$GITHUB_SHA s3://spdb-github-ci/
aws s3 cp $GITHUB_WORKSPACE/out/${{ matrix.name }}.log s3://spdb-github-ci/$GITHUB_SHA-${{ matrix.name }}.log
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use of uploading the logs to S3 after the fuzzer has already succeeded? I case of failure this job will not be executed anyway (because the previous job would fail), and in a successful run we have no use for the logs.

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 was told we want to save the logs, is this not correct? I can remove it if there's no use

Copy link
Contributor

Choose a reason for hiding this comment

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

The job later removes them, so they are not saved anyway. What we need is a way to store the logs on failure, not on success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the clean is only for testing, so that it will automatically clean itself instead of manually doing it. I'll change it to save only when the job fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the change, let me know if you want it different

@isaac-io isaac-io force-pushed the devops-ci/move_fuzz_to_reusable_workflow branch from 8b443c2 to 2a42fb1 Compare September 5, 2022 11:24

- name: fuzzersum
- name: Copy ${{ matrix.name }} logs to S3 due to failure
if: steps.${{ matrix.name }}.outcome != 'success'
Copy link
Contributor

Choose a reason for hiding this comment

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

This will run on cancellation, which we don't really want. You should probably only run this on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to run explicitly if the outcome is 'failure'

- name: clear s3 bucket
run: |
aws s3 rm s3://spdb-github-ci/ --recursive --include "*"
uses: speedb-io/speedb/.github/workflows/test_fuzz.yml@main
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the local repository syntax without pinning to main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@isaac-io isaac-io merged commit 8554fcf into main Sep 5, 2022
Yuval-Ariel pushed a commit that referenced this pull request Nov 23, 2022
Move the definition of the fuzz testing jobs to a separate workflow
so that it could be reused by workflows other than the main CI one.
Yuval-Ariel pushed a commit that referenced this pull request Nov 25, 2022
Move the definition of the fuzz testing jobs to a separate workflow
so that it could be reused by workflows other than the main CI one.
Yuval-Ariel pushed a commit that referenced this pull request Apr 30, 2023
Move the definition of the fuzz testing jobs to a separate workflow
so that it could be reused by workflows other than the main CI one.
udi-speedb pushed a commit that referenced this pull request Oct 31, 2023
Move the definition of the fuzz testing jobs to a separate workflow
so that it could be reused by workflows other than the main CI one.
udi-speedb pushed a commit that referenced this pull request Dec 3, 2023
Move the definition of the fuzz testing jobs to a separate workflow
so that it could be reused by workflows other than the main CI one.
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.

2 participants