-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add additional options to control storage endpoints on import #84
base: main
Are you sure you want to change the base?
Conversation
Also found (what seems to be) a bug in Compute ImageService.Create method: it won't accept URIs with ports, but this is exact form that is returned by endpoint resolver API. If you care, here are the details of backend error:
|
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.
Hi @kuzpactor,
Thanks for opening this PR, the code looks good to me, I suggested adding a check during the Configure
step for the post-processors, since this is intended to catch configuration errors before we even attempt to run the post-processor.
In the case of conflicting options such as the ones you're submitting here, I think it would be a good spot to do this check (or potentially warn that one will be ignored).
I'll let you decide what to do on those comments, and I'll re-review once you've had a chance to update.
Thanks again for the contribution!
p.config.StorageRegion = defaultStorageRegion | ||
} | ||
|
||
if p.config.StorageEndpointAutoresolve { |
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.
We should probably also check during Configure
that this is not set alongside StorageEndpoint
, the two look mutually exclusive, so we can error if both are set in the same config.
// StorageEndpoint custom Yandex Object Storage endpoint to upload image, Default `storage.yandexcloud.net`. | ||
StorageEndpoint string `mapstructure:"storage_endpoint" required:"false"` | ||
// StorageEndpointAutoresolve auto resolve storage endpoint via YC Public API ListEndpoints call. Option has | ||
// precedence over 'storage_endpoint' option. |
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.
If we reject using both at the same time through a check in Configure
(which I think would make sense), this comment should be amended to state that this and the storage_endpoint
options are mutually exclusive.
Similar to #43, PR adds options to control storage endpoints, but this time at the import part.
I guess it also finally closes #42.