Conversation
hildo
force-pushed
the
remove-guava-threadsafe
branch
from
7e782d0 to
e156a5e
Compare
|
Thanks for working on this @hildo, it would be great to see the removal of Guava completed. |
|
The dryrun is failing due to some changes that were made to Cedar Shared Integration Tests. Looking into it |
|
Thanks for checking @muditchaudhary ... i thought it ran locally for me fine, but I didn't get a chance to look into this further last night. Of course, if there's more work I need to do to address this, please let me know. |
|
Yeah, the change is very recent and now my local builds are failing too. The updated shared integration tests check for Duration and negative decimals. The tests are failing because we don't support these in CedarJava yet + there might be a bug for Entity tags. I am planning on adding the support soon which should fix the integration tests. Will keep you updated. Thanks for your contributions! |
|
No worries. Happy to help. If there's anything I can do to assist, please let me know. |
hildo
force-pushed
the
remove-guava-threadsafe
branch
from
e156a5e to
6dc221f
Compare
| */ | ||
| @JsonProperty("warnings") | ||
| public final ImmutableList<String> warnings; | ||
| public final List<String> warnings; |
There was a problem hiding this comment.
Would this count as a breaking change (hence requiring a major version bump) because we are updating type for a public field?
There was a problem hiding this comment.
In Rust SDK, PE is an experimental feature and hence does not obey SemVer. I'm not sure if CedarJava follows a similar rule given that there's no easy to specify an experimental feature. Just FYI.
There was a problem hiding this comment.
@muditchaudhary that is a good pickup. It would be a breaking change, especially if the application was using cedar-java's transitive dependencies to include guava (ImmutableList would not be available anymore). If the code stored this in a variable of type java.util.List, it wouldn't. But that's not an assumption that can be made.
I am not familiar with how the version numbers are managed. If we are to change them, is the change included in this PR? Or is the library versioned using another process? Happy to help where I am able to.
There was a problem hiding this comment.
I am assuming that if I am to make the version change, I should be setting the version at line 284 in CedarJava/build.gradle to 4.0.0? But I will wait for confirmation if that's the correct process
There was a problem hiding this comment.
@hildo If needed, we'll bump the version on release. So, there shouldn't be any action required within this PR. However, I am checking with others on how we'd want to approach it. Ideally, we want our major versions to be in-sync with Cedar, which is at 4.x. If we bump the major version for CedarJava for this change i.e., 5.x we won't be in-sync with Cedar versions anymore (which might cause some confusion). I'll provide an update soon.
We are also making a public field type change for AuthorizationResponse (which is not experimental)
|
Had a discussion with the team. It is preferred to not bump CedarJava major version for this change as it will make CedarJava versions out of sync with Cedar. Currently, we will leave this PR open and merge it when a major version bump is planned. Thank you again for the contribution. |
hildo
force-pushed
the
remove-guava-threadsafe
branch
from
6dc221f to
9cc606b
Compare
|
I just rebased my feature branch against main. Apologies if that means approvals get lost |
hildo
force-pushed
the
remove-guava-threadsafe
branch
from
9cc606b to
e4f5ad6
Compare
Signed-off-by: Ed Hillmann <edhillmann@yahoo.com>
Signed-off-by: Ed Hillmann <edhillmann@yahoo.com>
hildo
force-pushed
the
remove-guava-threadsafe
branch
from
e4f5ad6 to
776c064
Compare
5 participants
#86
Removed Guava. Previous attempts stalled over concerns about thread safety. These changes attempt to address those concerns by using threadsafe classes (like ConcurrentHashMap)