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

Issue 2146 main #2159

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Issue 2146 main #2159

merged 2 commits into from
Jan 10, 2024

Conversation

JustinKyleJames
Copy link
Contributor

@JustinKyleJames JustinKyleJames commented Jan 6, 2024

This fixes issue #2146. All tests passed.

In addition, I added two general test cases to test switching archive naming policy consistent->decoupled and decoupled->consistent after object had already been created. These were added for the following reasons:

  1. This is something we should be testing since the physical path in the DB for existing objects should override the archive naming policy.
  2. On my first attempt to fix 2146 I broke this behavior and realized there was no specific test for this.

See 03eb431 which fixes the scenario I was trying to fix with the open flags. I am now checking the openType. This is now working as expected.

Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Looks good.

s3/s3_operations.cpp Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Seems good overall

s3/s3_operations.cpp Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Show resolved Hide resolved
@JustinKyleJames JustinKyleJames marked this pull request as draft January 9, 2024 15:42
@JustinKyleJames JustinKyleJames marked this pull request as ready for review January 9, 2024 22:32
packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
s3/s3_operations.cpp Outdated Show resolved Hide resolved
s3/s3_operations.cpp Outdated Show resolved Hide resolved
@korydraughn
Copy link
Contributor

At the moment we don't have an automated way of running these tests in a topology environment.

We need to make sure testing of this plugin in that type of environment is enforced following the merging of this PR.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Good descriptive comments. I saw a couple of things in the comments for the test but overall this is solid.

packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
@trel
Copy link
Member

trel commented Jan 10, 2024

clang-format and trailing whitespace

good good

@JustinKyleJames
Copy link
Contributor Author

Note that after the latest changes I re-ran all tests and they passed.

@trel
Copy link
Member

trel commented Jan 10, 2024

ready for squashing, no pounds

@trel
Copy link
Member

trel commented Jan 10, 2024

issue numbers and content split look good to me. let's wait on one more vote.

Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Agreed. Split looks good. Pound it.

@JustinKyleJames
Copy link
Contributor Author

Agreed. Split looks good. Pound it.

Done

@alanking alanking merged commit 2f4b265 into irods:main Jan 10, 2024
1 of 2 checks passed
@JustinKyleJames JustinKyleJames deleted the issue_2146_2 branch January 10, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants