Oracle data volume following readonly convention#3908
Oracle data volume following readonly convention#3908danmoseley merged 3 commits intomicrosoft:mainfrom cmdkeen:update-oracle-datavolume
Conversation
Following convention on other database implementations
Updating public API
ghost
added
the
area-integrations
dotnet-policy-service
Bot
added
the
community-contribution
|
@dotnet-policy-service agree |
| public static IResourceBuilder<OracleDatabaseServerResource> WithDataVolume(this IResourceBuilder<OracleDatabaseServerResource> builder, string? name = null, bool isReadOnly = false) | ||
| => builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), "/opt/oracle/oradata", isReadOnly); |
There was a problem hiding this comment.
This will be a source breaking change in 8.1, right? Default parameters are fine to add, but it changes the default. If this change is important, perhaps we need it in 8.0
@eerhardt
There was a problem hiding this comment.
From my perspective the current default is hard broken - the default Oracle image does not come up if the data volume is mounted as read-only. I'll add a bit more detail on the linked issue.
There was a problem hiding this comment.
If the current default is "hard broken", why are we making it an option at all? If someone passes isReadOnly: true wouldn't they be just as broken?
There was a problem hiding this comment.
If it isn't possible for the mount to be read only we should remove the read only option entirely.
@DamianEdwards do you know if the other container types we have any issues with read only volumes?
There was a problem hiding this comment.
I believe some of them work if the volume already exists and has been initialized by a container of the same image already. The scenario is multiple containers using the same volume but only one has write access. That was the only reason we had an option for read only data volumes.
There was a problem hiding this comment.
OK thanks. Let me just confirm that also works with Oracle here and if it does we should keep the parameter (with isReadOnly = false as the default).
| /// <returns>The <see cref="IResourceBuilder{T}"/>.</returns> | ||
| public static IResourceBuilder<OracleDatabaseServerResource> WithDataVolume(this IResourceBuilder<OracleDatabaseServerResource> builder, string? name = null) | ||
| => builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), "/opt/oracle/oradata", true); | ||
| public static IResourceBuilder<OracleDatabaseServerResource> WithDataVolume(this IResourceBuilder<OracleDatabaseServerResource> builder, string? name = null, bool isReadOnly = false) |
There was a problem hiding this comment.
Are there any tests we can write for these APIs?
|
Feels important to get this for GA since it's a breaking change cc @mitchdenny |
|
@cmdkeen I updated your PR to remove the If/when we figure out readonly volume/bindmount support for Oracle then we can add it back in. And if someone is so inclined they can make this work themselves with the general purpose bind mount/volume methods. |
| /// <returns>The <see cref="IResourceBuilder{T}"/>.</returns> | ||
| public static IResourceBuilder<OracleDatabaseServerResource> WithDataVolume(this IResourceBuilder<OracleDatabaseServerResource> builder, string? name = null) | ||
| => builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), "/opt/oracle/oradata", true); | ||
| => builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), "/opt/oracle/oradata", false); |
There was a problem hiding this comment.
Nit, nice to name the parameter at the call site when passing a bool
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8871863329 |
|
@mitchdenny backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Adding readonly default false
Applying: Update PublicAPI.Unshipped.txt
Using index info to reconstruct a base tree...
A src/Aspire.Hosting.Oracle/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/Aspire.Hosting.Oracle/PublicAPI.Unshipped.txt deleted in HEAD and modified in Update PublicAPI.Unshipped.txt. Version Update PublicAPI.Unshipped.txt of src/Aspire.Hosting.Oracle/PublicAPI.Unshipped.txt left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Update PublicAPI.Unshipped.txt
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@mitchdenny an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Very happy, thanks for getting this in and sorted everyone. Absolutely agree that it is easy enough to do a read only volume via an underlying method if needed. |
6 participants
Closes #3907 - Oracle needs the data volume to be writeable. Following the convention on other database implementations to use a default false parameter.
Also fixes inconsistency between the readonly states of WithDataVolume and WithDataBindMount
Microsoft Reviewers: Open in CodeFlow