Fixing AzureCLI / CloudShell authentication #169
Conversation
|
@tombuildsstuff, |
- Hard-coding the value for `expiresIn` due to the Azure CLI and CloudShell authentication models having different types - Updating `expiresOn` to be the difference in seconds between the baseDate & now - rather than a date - Parsing the AzureCLI time out in the local system timezone, given it's saved in local time but with no timezone info
tombuildsstuff
force-pushed
the
azure-cli-funtime
branch
from
9e5e577 to
a04d94c
Compare
autorest/adal/cli.go
Outdated
|
|
||
| // ParseAzureCLIExpirationDate parses either a Azure CLI or CloudShell date into a time object | ||
| func ParseAzureCLIExpirationDate(input string) (*time.Time, error) { | ||
| log.Printf("[DEBUG] Token Date: %s", input) |
There was a problem hiding this comment.
Let's remove this line before we commit this.
autorest/adal/cli.go
Outdated
| ExpiresIn: strconv.Itoa(t.ExpiresIn), | ||
| ExpiresOn: t.ExpiresOn, | ||
| ExpiresIn: "3600", | ||
| ExpiresOn: strconv.Itoa(int(seconds)), |
There was a problem hiding this comment.
[nit] I think this would be more readable as difference.Seconds() and skipping the declaration of seconds
autorest/adal/cli.go
Outdated
| } | ||
|
|
||
| // ParseAzureCLIExpirationDate parses either a Azure CLI or CloudShell date into a time object | ||
| func ParseAzureCLIExpirationDate(input string) (*time.Time, error) { |
There was a problem hiding this comment.
Q: why do you return pointer to time.
time package uses value semantics by default, returning pointer causes one more allocation, moves object from stack to heap and creates work for garbage collector. as it is used only within a calling method it should return value.
this way it would stay on stack and get disposed when method finishes. no additional work for GC.
There was a problem hiding this comment.
I’ve opted for this approach since we’re returning nil for an invalid/unparseable date; given returning an empty object in addition to an error in the failure case feels misleading as a consumer of the API
There was a problem hiding this comment.
It seems like you use that pattern pretty religiously, @tombuildsstuff. I'm not really convinced either way, other than sorta general intuitiveness, can you build a case for why it's better to return a pointer? Personally, when I'm writing Go, I just treat retvals that accompany an error as garbage, it doesn't matter if the value is the default or memory with random bits. As you can imagine, in my workflow, changing the return type to a pointer has almost no consequence. What scenario am I missing where it's totally important? :)
edit: this isn't important enough for me to hold up a PR. Don't feel like this comment is holding you hostage.
There was a problem hiding this comment.
not particularly - I've only opted for this approach to force the consumer to handle the error case, rather than accidental comparing the values returned by the default object (it's more habit than anything else) - but totally happy to switch this out :)
tombuildsstuff
force-pushed
the
azure-cli-funtime
branch
from
727791b to
575eb82
Compare
autorest/adal/cli.go
Outdated
| @@ -1,16 +1,18 @@ | |||
| package adal | |||
There was a problem hiding this comment.
I thought about this a little this morning, and think that this code would be better off in a cli package. Particularly, I think it makes sense as a child of the azure package. In general, I'd like to have as few Azure specific references in the adal package as possible, that applies doubly to CLI specific code :)
There was a problem hiding this comment.
Now that I'm participating in writing this code, I think we should get @jhendrixMSFT to sign off as well.
marstr
dismissed
their stale review
Because I began contributing to this PR.
|
Talked with @jhendrixMSFT offline, he gave us the go-ahead to merge. |
expiresIndue to the Azure CLI and CloudShell authentication models having different typesexpiresOnto be the difference in seconds between the baseDate & now - rather than a date