Skip to content

Conversation

@Electronic-Waste
Copy link
Contributor

This PR adds fields and annotations to:

  • Tenant CRD
  • Warehouse CRD

cc👀 @flaneur2020 @hantmac

// Region of S3 storage.
Region string `json:"region,omitempty"`

// Endpoint of S3 storage.
Copy link

@flaneur2020 flaneur2020 Dec 12, 2024

Choose a reason for hiding this comment

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

add an optional for the endpoint? when you're using s3, endpoint is not an required parameter in most of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's optional now, since I added omitempty tag after the field.

Copy link

@flaneur2020 flaneur2020 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor Author

@Electronic-Waste Electronic-Waste left a comment

Choose a reason for hiding this comment

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

@flaneur2020 @hantmac I left some comments. Please let me know your ideas about them:)

Comment on lines +111 to +112
// Password encrypted with AuthType.
AuthString string `json:"authString,omitempty"`
Copy link
Contributor Author

@Electronic-Waste Electronic-Waste Dec 12, 2024

Choose a reason for hiding this comment

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

I was wondering, whether we need to add a field named AuthStringRef to reference Secret like what we do in S3 and meta configurations.

Copy link

@flaneur2020 flaneur2020 Dec 12, 2024

Choose a reason for hiding this comment

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

i feel having a reference to secret is nice, might be like support both of styles:

authString:
  value: "a md5 hash"
authString:
  secretRef:
    name: "blah"
    namespace: "xxx"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. What about maintaining consistency with S3 and Meta configs? I think it would be better if we add a new field named AuthStringRef. WDYT👀?

Choose a reason for hiding this comment

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

What about maintaining consistency with S3 and Meta configs? I think it would be better if we add a new field named AuthStringRef. WDYT👀?

looks good to me 👍

Comment on lines 108 to 110
// Time for Query clsuter to suspend when no query requests are received.
// +kubebuilder:default=1500
AutoSuspendAfterSecs int `json:"autoSuspendAfterSecs,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is not found in helm-charts. I'm also wondering whether we need to support this feature now or implement it in the next version of databend-operator.

Copy link
Collaborator

@hantmac hantmac left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@hantmac hantmac merged commit a7d5850 into databendcloud:main Dec 13, 2024
1 check passed
@Electronic-Waste Electronic-Waste deleted the feat/fill-crd branch December 17, 2024 07:50
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.

3 participants