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

improve s3 config project injection #901

Merged
merged 1 commit into from
Dec 18, 2024
Merged

improve s3 config project injection #901

merged 1 commit into from
Dec 18, 2024

Conversation

ddecrulle
Copy link
Contributor

No description provided.

@ddecrulle ddecrulle marked this pull request as ready for review December 18, 2024 08:21
@ddecrulle ddecrulle requested a review from garronej December 18, 2024 08:22
Comment on lines +85 to +94
disabled={
canInjectPersonalInfos
? false
: s3Config.origin === "deploymentRegion"
}
checked={
canInjectPersonalInfos || s3Config.origin !== "deploymentRegion"
? s3Config.isXOnyxiaDefault
: false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic should ideally be handled by the core. The component shouldn't need to be so aware of the intricacies of S3Config management; its responsibility should be limited to rendering the UI and forwarding the relevant user input.

That said, I understand the desire to avoid introducing another type just to adhere strictly to the doctrine, so I'll approve this impl.

@garronej garronej merged commit 645dd34 into main Dec 18, 2024
7 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