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

fix(secret): add a secret ref column to source #17269

Merged
merged 9 commits into from
Jun 17, 2024

Conversation

yuhao-su
Copy link
Contributor

@yuhao-su yuhao-su commented Jun 17, 2024

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 in StreamSourceInfo, but it should be used for format/encode option. We need another secret_ref for source.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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 in StreamSourceInfo, but it should be used for format/encode option. We need another secret_ref for source.

Add this to avoid backward compatibility issue

@yuhao-su yuhao-su added the need-cherry-pick-release-1.10 Open a cherry-pick PR to branch release-1.10 after the current PR is merged label Jun 17, 2024
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

@tabVersion
Copy link
Contributor

We had a secret_ref for source in StreamSourceInfo, but it should be used for format/encode option. We need another secret_ref for source.

Then let's unify them at the source level, pb source carries all secret mapping.

@yuhao-su
Copy link
Contributor Author

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.

@yuhao-su
Copy link
Contributor Author

Any way, I guess a migration script might be a better solution for those problem.

Copy link
Member

@yezizp2012 yezizp2012 left a 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.

src/meta/model_v2/migration/src/m20240525_090457_secret.rs Outdated Show resolved Hide resolved
src/meta/model_v2/migration/src/m20240525_090457_secret.rs Outdated Show resolved Hide resolved
src/meta/model_v2/src/source.rs Outdated Show resolved Hide resolved
@yuhao-su yuhao-su force-pushed the yuhao/add-secret-ref-to-source branch from 00d43ed to d13ed73 Compare June 17, 2024 19:36
@yuhao-su yuhao-su enabled auto-merge June 17, 2024 19:55
@yuhao-su yuhao-su added this pull request to the merge queue Jun 17, 2024
Merged via the queue into main with commit d5e10d8 Jun 17, 2024
32 of 34 checks passed
@yuhao-su yuhao-su deleted the yuhao/add-secret-ref-to-source branch June 17, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/run-e2e-single-node-tests ci/run-e2e-sql-migration-tests need-cherry-pick-release-1.10 Open a cherry-pick PR to branch release-1.10 after the current PR is merged type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants