Refactoring AccessToken Read method #341
Conversation
0ab43eb to
cbf3390
Compare
| "tokenId": { | ||
| "description": "The token identifier.", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Can you explain why we're removing this?
There was a problem hiding this comment.
The output tokenId is used as the resourceId, making this one useless
I see this in the description, I'm not sure I understand the implication.
There was a problem hiding this comment.
I mentioned in description:
- tokenId property is not used anywhere at all, lol. The output tokenId is used as the resourceId, making this one useless
There was a problem hiding this comment.
Yeah, as I mentioned in the thread, I don't really understand what that's supposed to mean. Why is it useless? What does it mean that it's not really used anywhere?
There was a problem hiding this comment.
I'll ask the question a different way - what do we (or our users) gain from making this breaking change?
There was a problem hiding this comment.
Oh, sorry, didn't update the page and never saw second comment ':D
So the property is literally not used. It's never filled out on create or update, never read from properties or anything. I think it was added originally, then author of AccessToken pivoted to storing tokenId in the resourceId and never cleaned up. That's just a guess as to why though.
We can leave it in to avoid breaking change, no problem with that really, except if a customer goes and tried to access the property and it's empty. Alternatively I can actually fill it out, but that would be storing the same data in 2 places.
So I think options are:
- remove (my preference, breaking change, but not going to impact customers)
- fill (redundant data storing)
- ignore (do nothing, leave it as empty property)
There was a problem hiding this comment.
Thanks for explaining! Agree with you, makes sense to remove it.
| inputMap := resource.PropertyMap{} | ||
| inputMap["description"] = resource.NewPropertyValue(input.Description) | ||
| outputStore := resource.PropertyMap{} | ||
| outputStore["__inputs"] = resource.NewObjectProperty(inputMap) |
There was a problem hiding this comment.
Should we stop doing this __inputs thing like we did for webhooks: #87
As described in the PR, we don't do this __inputs thing in ay other providers and have also been refactoring our code to move away from it in this repo.
There was a problem hiding this comment.
I can do that, was trying to keep refactor just to the Read method fix
There was a problem hiding this comment.
Totally cool to leave as-is for now, but let's open an issue to follow-up?
Summary
tokenIdproperty is not used anywhere at all, lol. The output tokenId is used as the resourceId, making this one uselessTesting