-
Notifications
You must be signed in to change notification settings - Fork 590
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
fix(secret): add a secret ref column to source #17269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should not modify an existing migration, just create a new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yezizp2012 said it's ok since it's not released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more to change. secret_ref should not be map<string, u32>. We need more than a secret id like if it's an as file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more to change. secret_ref should not be map<string, u32>.
Here is a little diff with my assumption, ref secret <secret-name>
means the source/sink relies on the secret binary. Seeing it as a string, or a file, is something more related to the exec itself rather than spec in catalog.
Just share my thoughts, it is okay you impl in your way.
Then let's unify them at the source level, pb source carries all secret mapping. |
How do you tell the difference between secrets for source and encode/format when they have the same name. |
Any way, I guess a migration script might be a better solution for those problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, need to ensure that it will be included in release 1.10.
…hao/add-secret-ref-to-source
00d43ed
to
d13ed73
Compare
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
We had a
secret_ref
for source inStreamSourceInfo
, but it should be used for format/encode option. We need another secret_ref for source.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.
We had a
secret_ref
for source inStreamSourceInfo
, but it should be used for format/encode option. We need another secret_ref for source.Add this to avoid backward compatibility issue