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.

Sign up for GitHub

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

Jump to bottom

[irods/irods#3727] Remove ASSERT_* (and bzero) usage (main) #2027

Merged
merged 6 commits into from
Apr 5, 2022
Merged

[irods/irods#3727] Remove ASSERT_* (and bzero) usage (main) #2027

merged 6 commits into from
Apr 5, 2022

Conversation

alanking
Copy link
Contributor

Tests need to be run

@alanking
Copy link
Contributor Author

companion PR to irods/irods#6155

@alanking alanking force-pushed the 3727.m branch 2 times, most recently from 1e8571e to 8f4ad03 Compare March 10, 2022 16:02
@alanking alanking marked this pull request as ready for review March 10, 2022 16:03
@alanking
Copy link
Contributor Author

Putting this back in draft. I do'nt think I actually finished it

@alanking alanking marked this pull request as draft March 11, 2022 18:27
@alanking
Copy link
Contributor Author

This change depends on #2041

@trel
Copy link
Member

trel commented Mar 15, 2022

#2041 is now merged

@alanking alanking marked this pull request as ready for review March 15, 2022 20:26
@alanking
Copy link
Contributor Author

Confirmed that this builds, haven't run tests yet.

@alanking
Copy link
Contributor Author

The test hook and suite need some work in order to run with python3 for main, so I will open a separate PR based on this branch for that. Unfortunately, main no longer builds because of use of the deprecated ASSERT_* macros, so we need to get this in first. I'll update once I get tests to run

@alanking
Copy link
Contributor Author

Okay, went with a different plan :) The latest force-push should be treated as a brand new set of changes, because they are.

In addition to removing ASSERT_* from the codebase, this changeset allows the test suite to be run in parallel in the testing environment. I have shown that 3 of the 6 test suites in test_irods_resource_plugin_s3_minio pass when run in parallel (for some reason) but all of them pass when run serially. I will create a new issue for this. I'm running the test suite one more time to make sure.

@alanking
Copy link
Contributor Author

I confirmed that all of the tests passed. There was an interaction between the tests which caused some to fail, but I've captured this in #2057

JustinKyleJames
JustinKyleJames approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@JustinKyleJames JustinKyleJames left a comment

Choose a reason for hiding this comment

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

This all looks straight-forward.

JustinKyleJames
JustinKyleJames suggested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@JustinKyleJames JustinKyleJames left a comment

Choose a reason for hiding this comment

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

I had a couple of comments where I agreed with the comments in the code. Please remove those code sections as requested.

s3/libirods_s3.cpp Outdated Show resolved Hide resolved
s3/libirods_s3.cpp Show resolved Hide resolved
@alanking
Copy link
Contributor Author

alanking commented Apr 5, 2022

Addressed the comments!

@alanking alanking mentioned this pull request Apr 5, 2022
Merged
@JustinKyleJames
Copy link
Contributor

Addressed the comments!

Looks good to me.

alanking added 6 commits April 5, 2022 12:10
@alanking
[irods#2048] Bump externals in build hook
835dc3e 
Trimmed down externals installed as these are provided in irods-dev and
irods-runtime packages or were not needed for this plugin.
@alanking
[irods#2053][irods#2056][irods/irods#6020] Fixes for test hook
2a6a6d0 
Add --test, --skip-setup, and --teardown-minio options to test hook.

--skip-setup skips downloading and starting the MinIO service which
means that the user running the test assumes that MinIO is already
running in the configuration expected by the tests.

--teardown-minio will terminate the MinIO processes after running the
tests. --skip-setup and --teardown-minio are incompatible parameters
because the process object returned from subprocess.Popen must be
available to execute the termination logic, which is only the case when
setup is performed on the same test hook invocation.

Also replaced usage of deprecated features of MinIO:

- Replaced usage of MINIO_ACCESS_KEY and MINIO_SECRET_KEY with
MINIO_ROOT_USER and MINIO_ROOT_PASSWORD

- Started using --console-address to explicitly specify the API port for
the MinIO server

Also uses python3 in the test hook.
@alanking
[irods#1806] Fix include path for rodsLog.h
ccec66c
@alanking
[irods#2042] Remove use of bzero
a986206
@alanking
[irods/irods#3727] Remove ASSERT_* from libirods_s3
d6805ee
@alanking
[irods/irods#3727] Remove ASSERT_* from s3_operations
b8d6f84
@alanking
Copy link
Contributor Author

alanking commented Apr 5, 2022

Squashed and #'d and mergin

@alanking alanking merged commit 42b111b into irods:main Apr 5, 2022
@alanking alanking deleted the 3727.m branch April 5, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinKyleJames JustinKyleJames JustinKyleJames requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alanking @trel @JustinKyleJames