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

Convert ObjectInformation to a builder #689

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

MrCreosote
Copy link
Member

Pain now = makes it easier to add the admin metadata and any other future new fields

Pain now = makes it easier to add the admin metadata and any other
future new fields
@@ -239,54 +188,10 @@ private Reference getLast(final List<Reference> refpath) {
return refpath.get(refpath.size() - 1);
}

@Override
public String toString() {
Copy link
Member Author

Choose a reason for hiding this comment

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

toString() is too much of a pain to test given how easy it is to just add it in temporarily when you need it.

@@ -18,8 +18,6 @@ public class UncheckedUserMetadata {

private Map<String, String> metadata;

//TODO CODE empty constructor -> empty metadata
Copy link
Member Author

Choose a reason for hiding this comment

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

YAGNI

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #689 (4519cf9) into develop (fa4f045) will increase coverage by 0.12%.
Report is 1 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #689      +/-   ##
=============================================
+ Coverage      87.29%   87.41%   +0.12%     
+ Complexity      5221     5189      -32     
=============================================
  Files            223      223              
  Lines          17367    17312      -55     
  Branches        2578     2545      -33     
=============================================
- Hits           15161    15134      -27     
+ Misses          1738     1711      -27     
+ Partials         468      467       -1     

@MrCreosote
Copy link
Member Author

Not sure why the one test is failing at the set up stage consistently while the other isn't... will look into it tomorrow

@ialarmedalien
Copy link
Collaborator

The issue with the set-up is due to the version of minio not being available from the supplied link -- the issue is intermittent (maybe a load balancer on the minio website and different dir structures on the different servers behind it?) and I found the quickest and easiest way to fix it was just to add the relevant release of minio to the workspace repo.

go to https://dl.minio.io/server/minio/release/linux-amd64/archive/
search for minio.RELEASE.2019-05-23T00-29-34Z -- nowhere to be found (at least some of the time)
it's now in https://dl.minio.io/server/minio/release/linux-amd64/archive/minio-2019-releases.tar.gz

chksum == null ||
size < 1) {
throw new IllegalArgumentException("One or more of the required arguments are "
+ "not set. Please check the documentation for the builder.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to be extra accurate in your error messaging...

"One or more of the required arguments are invalid or not set."

(objectId / size / version are set, they just don't pass the tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand - the with methods prevent setting invalids values. If a value has been set at all it must be valid.

-1 just signifies the value has never been set, maybe that's the confusion?

Copy link
Collaborator

@ialarmedalien ialarmedalien left a comment

Choose a reason for hiding this comment

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

LGTM.

@MrCreosote
Copy link
Member Author

Looks like the tests finally passed

@MrCreosote MrCreosote merged commit 756aef4 into kbase:develop Oct 11, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants