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

Issues 2151 2153 #2155

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Issues 2151 2153 #2155

merged 1 commit into from
Dec 19, 2023

Conversation

JustinKyleJames
Copy link
Contributor

This addresses the istream append issues.

I put this on top of #2152. Once that is merged this will be rebased.

All tests passed.

@JustinKyleJames JustinKyleJames force-pushed the issues_2151_2153 branch 2 times, most recently from e5fafb6 to 2ff4592 Compare December 14, 2023 12:53
@JustinKyleJames JustinKyleJames mentioned this pull request Dec 14, 2023
Merged
@alanking
Copy link
Contributor

@JustinKyleJames - #2152 is now merged. Let's go ahead and rebase.

alanking
alanking reviewed Dec 14, 2023
View reviewed changes
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.

Looks good overall. Had a couple of pretty minor suggestions.

s3/s3_operations.cpp Outdated Show resolved Hide resolved
s3/s3_transport/include/s3_transport.hpp Outdated Show resolved Hide resolved
alanking
alanking reviewed Dec 14, 2023
View reviewed changes
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.

Saw one more thing - take it or leave it. This seems good to me. Maybe wait for one more reviewer before we put it in.

s3/s3_operations.cpp Outdated Show resolved Hide resolved
korydraughn
korydraughn reviewed Dec 18, 2023
View reviewed changes
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 overall.

packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
@korydraughn
Copy link
Contributor

Will review once the latest changes appear.

alanking
alanking reviewed Dec 19, 2023
View reviewed changes
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.

Couple of lingering things then we can get this in

packaging/resource_suite_s3_nocache.py Outdated Show resolved Hide resolved
packaging/resource_suite_s3_nocache.py Show resolved Hide resolved
alanking
alanking approved these changes Dec 19, 2023
View reviewed changes
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.

Nice. Please squash to taste and add #s.

@JustinKyleJames
[irods#2151][irods#2153] Stat cache file if it is open
19ecdba 
If there is an open cache file, stat it instead of doing a HEAD to S3.
Fix translation of open mode to posix stream.
@JustinKyleJames
Copy link
Contributor Author

Nice. Please squash to taste and add #s.

Done

@alanking alanking merged commit 97940fd into irods:main Dec 19, 2023
0 of 2 checks passed
@JustinKyleJames JustinKyleJames mentioned this pull request Dec 20, 2023
Merged
@JustinKyleJames JustinKyleJames deleted the issues_2151_2153 branch December 21, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korydraughn korydraughn korydraughn left review comments

@alanking alanking alanking approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

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