-
Notifications
You must be signed in to change notification settings - Fork 15
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
Workflow to run S3 tests. Fixes #782 #809
Conversation
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
==========================================
- Coverage 34.35% 34.05% -0.30%
==========================================
Files 61 61
Lines 10483 10472 -11
==========================================
- Hits 3601 3566 -35
- Misses 6481 6505 +24
Partials 401 401 see 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
.github/workflows/s3-tests.yml
Outdated
|
||
- name: Build neofs-s3-gw docker image | ||
run: | | ||
cp .github/test-env .env #looks ugly, but I want to try |
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.
Does make require an .env 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.
You're right, it doesn't
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.
You're right, it doesn't
If not needed, then why cp .github/test-env .env
?
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.
Now, I removed it
.github/workflows/s3-tests.yml
Outdated
|
||
- name: Prepare s3tests config | ||
run: | | ||
sed -i -e "s/host =.*$/host = ${S3_GW_IP}/" ${S3_TESTS_CONFIG} |
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.
Dangerous regular expression - maybe there's more than one string that matches the host =.*
pattern
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.
By default, sed uses only single-line matching. To enable multiline matching you need to enable it with 'N' command, like this:
sed 'N; s/.* MULTILINE TEXT.*//' test.file
So, greedy regex is pretty safe here.
But, ok. I switched to a non-greedy '.*?' pattern
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.
It is best to replace lines like this with:
sed -i -E "s/host =.*?$/host = ${S3_GW_IP}/" ${S3_TESTS_CONFIG}
with the following:
sed -i -E "s/host =__HOSTS_TO_CHANGE__/host = ${S3_GW_IP}/" ${S3_TESTS_CONFIG}
Of course, this will require changes to the config file too.
That way you can be sure that some other line that someone else put in the config file won't change.
It will also make it easier for other users to interact with the config 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.
Go rid of this sed substitution with suitable employment of .env file for s3-dev-env.
cb3589f
to
2eec379
Compare
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.
It might be a good idea to fix regular expressions
Signed-off-by: Maksim Gelbakhiani <max@nspcc.io>
2eec379
to
ee332da
Compare
I've just noticed that we're completely missing expirations here, they should be added |
Signed-off-by: Maksim Gelbakhiani <max@nspcc.io>
cf6d42d
to
e03a17b
Compare
Merge after
nspcc-dev/s3-tests#34
nspcc-dev/s3-tests#33
After merging we'll need to add action secrets:
secrets.TEST_RESULTS_WALLET
secrets.TEST_RESULTS_PASSWORD
And action variables:
vars.TEST_RESULTS_NEOFS_NETWORK_DOMAIN
vars.TEST_RESULTS_CID
vars.TEST_RESULTS_HTTP_GATE
SSL is going to be fixed on later stages