-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Modify scope alias map to match the one on the official document #8496
Conversation
Hey thanks for this. We generate this library from https://github.com/GoogleCloudPlatform/magic-modules so I can go ahead and upstream this change tomorrow or if you want to make a pull request against that repository directly it might be slightly faster. Either way, I'll try to get this in soon for ya :) |
@ScottSuarez Thanks. I hope this PR works. |
@@ -26,8 +26,7 @@ func canonicalizeServiceScope(scope string) string { | |||
"storage-ro": "https://www.googleapis.com/auth/devstorage.read_only", | |||
"storage-rw": "https://www.googleapis.com/auth/devstorage.read_write", | |||
"taskqueue": "https://www.googleapis.com/auth/taskqueue", | |||
"trace-append": "https://www.googleapis.com/auth/trace.append", | |||
"trace-ro": "https://www.googleapis.com/auth/trace.readonly", | |||
"trace": "https://www.googleapis.com/auth/trace.append", |
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.
Thanks for upstreaming this, sorry for the late feedback but I'm hesitant to make this change as we might break the users of trace-append and trace-ro. Do these fields not work at all as is? @rileykarson
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.
From my understanding this is just an alias. So could we not just add trace as an additional potion for trace.append instead of removing the other two
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.
Thank for reviewing, @ScottSuarez. Yes, as you say, it is just an alias but our official document only mentions "trace" as the alias for https://www.googleapis.com/auth/trace.append
, so terraform should support that alias as well.
I confirmed that "https://www.googleapis.com/auth/trace.readonly" is used still though it is not listed our Google Compute Engine doc and OAuth scope doc.
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.
Odd- https://cloud.google.com/trace/docs/reference/v1/rest/v1/projects.traces/get
includes trace.readonly
, but it's disappeared elsewhere. It was added in hashicorp/terraform#14633.
Can we add this new scope but leave the others & file an issue to remove them in 4.0.0
? I'd rather not remove from the list outside a major release.
closing this as we have the upstreamed topic |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes #8495