-
Notifications
You must be signed in to change notification settings - Fork 213
Client 3898 go cov pipeline #583
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
base: stage
Are you sure you want to change the base?
Conversation
c74cc84 to
198fbd9
Compare
khaf
left a comment
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.
There are some issues in the workflow and test infra which should be addressed.
| docker run --rm --network host aerospike/aerospike-tools "$@" | ||
| } | ||
|
|
||
| while true; do |
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.
This will hang indefinitely if the server does not start for some reason.
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.
Some kind of timeout mechanism should be implemented here, maybe for 30s or other acceptable duration.
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 guess git step have some default timeout if such thing happen ,
but it make sense to have our own timeout, we can configure at git step level which would be more cleaner
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 would recommend timeout with exponential and multiplied delay on every iteration. Cap at certain number of intervals. I would make timeout and number of intervals configurable via actions vars.
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
| run: | | ||
| echo "Go version: ${{ steps.get-go-version.outputs.go-version }}" | ||
| echo 'Matrix output:' | ||
| echo '${{ steps.create-server-matrix.outputs.matrix }}' | jq . |
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.
here as well.
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
.github/workflows/build.yml
Outdated
| uses: ./.github/workflows/merge-coverage.yml | ||
| with: | ||
| artifact_pattern: cover_* | ||
| threshold: 65 |
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.
Same here.
| with: | ||
| name: coverage-html | ||
| path: coverage-reports | ||
| if-no-files-found: error No newline at end of file |
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.
And here.
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
| serverCommand := types.Ternary( | ||
| serverVersion.IsGreaterOrEqual(version.ServerVersion_8_1), | ||
| "release", | ||
| serverVersion.IsGreaterOrEqual(version.ServerVersion_8_1), |
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.
No such key available as of v8.1.0.1. Should be reverted.
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.
PR has already been created here -#584
.github/config/aerospike-sc.conf
Outdated
| @@ -0,0 +1,68 @@ | |||
| # Aerospike database configuration file | |||
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.
To enable sc mode all you have to do is aerolab conf sc. In fact anything you need to change with the conf you should be able to do with aerolab conf....
❯ al conf
Usage:
aerolab [OPTIONS] conf [command]
Global Options:
--beep cause the terminal to beep on exit; if specificied multiple times, will be once on success and >1 on failure
--beepf like beep, but does not trigger beep on success, only failures
Available commands:
generate Generate or modify Aerospike configuration files
sc Configure the cluster to use strong-consistency, with roster and optional RF changes
fix-mesh Fix mesh configuration in the cluster
rackid Change/add rack-id to namespaces in the existing cluster nodes
namespace-memory Adjust memory for a namespace using total percentages
adjust Adjust running Aerospike configuration parameters
help Print help
Unless aerolab conf... does not allow you to set an option I would not create configurations file.
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 tried creating a cluster using Aerolab and then running the aerolab conf sc command, but it didn’t work. Although the configuration appears to be updated when inspecting the node, the cluster does not actually operate in SC mode, and the test cases fail. However, when I create the cluster by providing a custom configuration upfront, the cluster operates correctly in SC mode, and all the test cases pass. Even with aerolab if you use aerolab cluster create -v 8.1.0.1 -n aero-sc -o /.github/config/aerospike-sc.conf then this works but we need to pass config file upfront
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.
Additionally, the configuration requires the flag strong-consistency-allow-expunge true without it, some test cases particularly delete operation fail with the error Operation not allowed this time. This setting, when enabled after cluster creation, does not take effect as expected. It only works correctly when supplied upfront as part of config file during cluster creation.
default_json: |
{
"stable": { "version": "8.1.0.1" },
"release": { "version": "8.1.0.1_2" }
}
Also if we use aerolab to download above version during single node deployment, the release one will fail as they are docker tags..but not present in https://download.aerospike.com/artifacts/aerospike-server-enterprise/ which aerolab uses to fetch the artifact from
.github/workflows/build.yml
Outdated
| jobs: | ||
| make-matrix: | ||
| runs-on: ubuntu-latest | ||
| runs-on: ubuntu-latest |
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 would not use lates version of anything. Things start breaking for the moment new disto is out. I would pin to a version. Also make this configurable via vars. Take a look at this line
I would make sure to fix this in all the workflows you have added/updated.
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
39beca3 to
acc89c2
Compare
dfd3a87 to
41034c4
Compare
41034c4 to
98b649d
Compare
merge coverageworkflow generating merged coverage report across all job happened within a workflowcoverage-htmlfolder from the uploaded artifact and can do following thingsindex.htmlfile to see the uncovered linesNote :
reugn/github-action-aerospike@v1is unusable for single-node EE cluster setup because of bugs.