-
Notifications
You must be signed in to change notification settings - Fork 24
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: restore bucket_endpoint #351
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.
LGTM
main.tf
Outdated
@@ -164,6 +164,7 @@ resource "ibm_cos_bucket" "cos_bucket1" { | |||
resource_instance_id = local.cos_instance_id | |||
region_location = var.region | |||
cross_region_location = var.cross_region_location | |||
endpoint_type = var.bucket_endpoint |
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.
bucket_endpoint seems misleading to a consumer, bucket_endpoint_type would be more meaningful (also because used to fill endpoint_type attribute)
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.
This was restored as it was before to match pre-existing usage.
If we want to change the name, maybe something like management_endpoint_type_for_bucket to try and highlight that it is not a bucket endpoint type, but a management endpoint type for buckets. Thoughts?
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.
As it has been restored to pre-existing usage I removed the request for changes.
However yes your proposal makes it more meaningful
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 can call it whatever we like now I guess since it was actually removed, so it wont be a breaking change. I'm easy on the naming, but I think the main thing we need to do is ensure the variable description is clear
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.
After various discussions, I will change the name of the variable so that is more obvious what it does.
variables.tf
Outdated
@@ -119,6 +119,16 @@ variable "bucket_storage_class" { | |||
} | |||
} | |||
|
|||
variable "bucket_endpoint" { |
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.
see comment above for main.tf
variables.tf
Outdated
@@ -119,6 +119,16 @@ variable "bucket_storage_class" { | |||
} | |||
} | |||
|
|||
variable "bucket_endpoint" { | |||
description = "The type of endpoint for the IBM provider to use to manage the bucket. (public, private, direct)" |
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.
IBM provider -> IBM terraform provider
0fdcd08
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.
ok for me
👍 |
🎉 This PR is included in version 6.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Restore bucket_endpoint which was removed in version 6.0.0. This value is used by the provider to construct the url used to access cos management apis.
#344
Types of changes in this PR
No release required
Release required
x.x.X
): Change that fixes an issue and is compatible with earlier versions)x.X.x
): Change that adds functionality and is compatible with earlier versions)X.x.x
): Change that is likely incompatible with previous versions)Release notes content
There are not breaking changes included in this fix.
Checklist for reviewers
Merge actions for mergers
Merge by using "Squash and merge".
Use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author.
The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).