-
Notifications
You must be signed in to change notification settings - Fork 96
fix(cli/rest) Support Glue REST operations with Iceberg-Go CLI #459
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
base: main
Are you sure you want to change the base?
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.
I'm doubtful about this approach.
Can we pass this via flags?
This makes the CLI less unified. I think it's better to keep this approach consistent — by using flags.
Thanks @laskoviymishka for reviewing ! Yes, that was my initial approach as well to make CLI args and config files to be as close as possible. Since we may end up in passing too many arguments, i used this approach. Do you think adding these REST config arguments as a JSON would be easier ? |
I think this is still a problem. Currently, using a custom JSON schema adds an unnecessary layer of complexity - it's less clear to the user and makes maintenance more cumbersome. Could we consider following the pattern used by other tools in the ecosystem? See for example Iceberg Catalog Migrator
Another option is simply by-pass this as |
I agree with @laskoviymishka that we shouldn't be passing custom JSON stuff and should instead use flags or just follow whatever pattern pyiceberg is using. |
Thank you @laskoviymishka @zeroshade for the suggestion. I have updated the code to follow the pattern in Iceberg Catalog Migrator. Successfully tested cli using command below,
|
output: json | ||
credential: client-id:client-secret | ||
warehouse: 123456789012 | ||
rest-config: auth-url=https://auth.example.com,sigv4-enabled=true,sigv4-region=us-east-1,sigv4-service=glue,tls-skip-verify=false |
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.
I'm confused, why don't we just use the existing properties for these that we already recognize as separate properties to pass?
i.e. rest.sigv4-enabled
, rest.signing-region
, rest.authorization-url
, etc.. (see catalog/rest.go
)
Motivation
To Support AWS Glue Iceberg endpoint using Iceberg-Go CLI.
Forbidden error
is thrown when Iceberg tables in glue are queried using CLI.https://docs.aws.amazon.com/glue/latest/dg/connect-glu-iceberg-rest.html
Fix:
RestCatalogConfig
struct toCatalogConfig
structRestCatalogConfig
contains rest catalog configuration properties.Testing:
rest_config.yaml
to query Iceberg tables in AWS Glue.