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

Rename config properties in Google Sheets #15042

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Nov 16, 2022

Description

Rename config properties in Google Sheets

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Google Sheets
* Rename config properties from `credentials-path` to `gsheets.credentials-path`, 
  from `metadata-sheet-id` to `gsheets.metadata-sheet-id`, 
  from `sheets-data-max-cache-size` to `gsheets.max-data-cache-size` and 
  from `sheets-data-expire-after-write` to `gsheets.data-cache-ttl`. ({issue}`15042`)

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Why? The connector properties already live in separate namespace from other connectors.

@ebyhr
Copy link
Member Author

ebyhr commented Nov 16, 2022

For consistency with other connectors. I suppose we use such prefixes regardless of namespace conflict.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comment.

Consistency makes sense but personally I think moving in the less verbose direction is cleaner. I don't feel too strongly about it.

However your change matches the majority of connectors - trino-base-jdbc is the big one which doesn't use prefixes but almost everything else does.

@@ -46,10 +46,10 @@ public void testExplicitPropertyMappings()
Path credentialsFile = Files.createTempFile(null, null);

Map<String, String> properties = ImmutableMap.<String, String>builder()
.put("credentials-path", credentialsFile.toString())
Copy link
Member

Choose a reason for hiding this comment

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

Add a testLegacyConfigs?

@ebyhr ebyhr merged commit b05d6f0 into trinodb:master Nov 17, 2022
@ebyhr ebyhr deleted the ebi/sheets-config branch November 17, 2022 02:20
@ebyhr ebyhr mentioned this pull request Nov 17, 2022
@github-actions github-actions bot added this to the 404 milestone Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants