-
Notifications
You must be signed in to change notification settings - Fork 151
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 Optional Expiry to Auth Tokens #844
Conversation
b1ae8dc
to
edcd608
Compare
If you would like the make the compile errors go away, and allow tests to run or allow other developers to clone + build and test this, you can run + commit
This will tell go to use your draft version of go-tfe from that commit. It will need to be updated to an actual released go-tfe version before this can be merged....but it will need to be updated regardless when the actual version of go-tfe is released, so 🤷 no extra work needed by pointing to an unreleased version of go-tfe in the mean time |
59008e1
to
42b96e5
Compare
42b96e5
to
a88c677
Compare
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.
One easy fix in the docs, and a few suggestions on cleaning up the code a bit.
If the requested tests are a bigger lift than I am thinking, let me know!
a88c677
to
52c7db1
Compare
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.
looking good to me, just one more small error in the docs
729132e
to
ddb7c52
Compare
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 LGTM!
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.
I wonder if we should suggest such a dramatically long expiration period in the docs (I think it's 28 years) ... and more generally I wonder if there is more guidance we can give for generating this format in particular.. Example: You can generate this string using the date
tool in unix systems.. unfortunately this tool has different flags in linux and darwin and other systems:
darwin:
date -Iseconds -v"+90d"
linux:
date -Iseconds -d"+90 days"
Or you can use a terraform function that doesn't make it easy to work with long durations:
$ terraform console
> timeadd(timestamp(), "2160h")
"2023-08-28T23:04:47Z"
To open up a slightly broader discussion... I don't believe that this format is perfectly suitable for this use case because it must be static to be useful and doesn't lend itself to human readability. What I mean by static is that it would not make sense to invoke a terraform function like timestamp() to define this value because it would force a re-create on the resource every time it was evaluated. Nor is there an easy set of terraform functions to format a more human readable date... We expect users to statically define an expiration date in iso8601 format, which is hard to read and hard to write. So what if we chose a more human readable format that was also easy to produce? Like RFC 5322 (email format)
$ date -v"+90d" -R
Mon, 28 Aug 2023 17:10:48 -0600
This can be reliably parsed with go, looks nice in config, and we could even document the terraform format string for the expected format (it's EEE, DD MMM YYYY hh:mm:ss ZZZZ
)
$ terraform console
> formatdate("EEE, DD MMM YYYY hh:mm:ss ZZZZ", timeadd(timestamp(), "2160h"))
Tell me what you think.
/cc @JarrettSpiker
@brandonc: @juliannatetreault and I discussed this, and definitely agree that we can improve the documentation with some more realistic examples, and provide the user with some commands they can use to generate an appropriate timestamp.
I don't know that I totally agree here though. Maybe it is personal preference but I don't find And I think I would actually prefer to write the I would be open to changing the format if there are strong feelings about it. The current format being alright could be just personal preference though...and with all things being equal I lean towards using the format that the API will be using. On a related note, just googling around I found the time provider...do you think that that would be something worth recommending as a way to generate this timestamp? Something like:
|
@JarrettSpiker @juliannatetreault I am super into recommending the time provider in the documentation! I wouldn't mind keeping iso8601 if it makes the examples this simple! Plus, it has the added benefit of being the same standard format that timestamp(), etc. uses.
|
ddb7c52
to
d7202b9
Compare
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.
👇
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.
I made a mistake!
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.
Very close 🎉
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.
WTG
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tfe](https://registry.terraform.io/providers/hashicorp/tfe) ([source](https://togithub.com/hashicorp/terraform-provider-tfe)) | required_provider | minor | `0.45.0` -> `0.46.0` | --- ### ⚠ Dependency Lookup Warnings ⚠ Warnings were logged while processing this repo. Please check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>hashicorp/terraform-provider-tfe (tfe)</summary> ### [`v0.46.0`](https://togithub.com/hashicorp/terraform-provider-tfe/blob/HEAD/CHANGELOG.md#v0460-July-3-2023) [Compare Source](https://togithub.com/hashicorp/terraform-provider-tfe/compare/v0.45.0...v0.46.0) FEATURES: - **New Resource**: `r/tfe_agent_pool_allowed_workspaces` restricts the use of an agent pool to particular workspaces, by [@​hs26gill](https://togithub.com/hs26gill) [870](https://togithub.com/hashicorp/terraform-provider-tfe/pull/870) - `r/tfe_organization_token`: Add optional `expired_at` field to organization tokens, by [@​juliannatetreault](https://togithub.com/juliannatetreault) (#​[hashicorp/terraform-provider-tfe#844)) - `r/tfe_team_token`: Add optional `expired_at` field to team tokens, by [@​juliannatetreault](https://togithub.com/juliannatetreault) (#​[hashicorp/terraform-provider-tfe#844)) - `r/tfe_agent_pool`: Add attribute `organization_scoped` to set the scope of an agent pool, by [@​hs26gill](https://togithub.com/hs26gill) [870](https://togithub.com/hashicorp/terraform-provider-tfe/pull/870) - `d/tfe_agent_pool`: Add attribute `organization_scoped` and `allowed_workspace_ids` to retrieve agent pool scope and associated allowed workspace ids, by [@​hs26gill](https://togithub.com/hs26gill) [870](https://togithub.com/hashicorp/terraform-provider-tfe/pull/870) BUG FIXES: - `r/tfe_workspace_run`: Ensure `wait_for_run` correctly results in a fire-and-forget run when set to `false`, by [@​lucymhdavies](https://togithub.com/lucymhdavies) (#​[hashicorp/terraform-provider-tfe#910)) - `r/tfe_workspace_run`: Fix rare random run failures; adjust lists of expected run statuses to ensure that a plan is completely processed before attempting to apply it, by [@​uk1288](https://togithub.com/uk1288) (#​[hashicorp/terraform-provider-tfe#921)) - `r/tfe_notification_configuration`: Add support for missing "Check failed" Health Event notifications, by [@​lucymhdavies](https://togithub.com/lucymhdavies) (#​[hashicorp/terraform-provider-tfe#927)) - `r/tfe_registry_module`: Fix a bug that prevented users from being able to create a registry module using a github app, by [@​dsa0x](https://togithub.com/dsa0x) (#​[hashicorp/terraform-provider-tfe#935)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNTkuNCIsInVwZGF0ZWRJblZlciI6IjM1LjE1OS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: astronaut-panda[bot] <137164246+astronaut-panda[bot]@users.noreply.github.com>
Description
Allow an optional
expired_at
to be set on authentication API tokens via the provider.Testing plan
expired_at
field still work as expected.expired_at
can be set on organization and team API tokens and that once set, the expiration date displays in the UI (/app/org-name/authentication-tokens
) with the expiration date that was provided.expired_at
cannot be set on organization and team API tokens and that if attempted to be set, an error is returned.External links
Output from acceptance tests
resource_tfe_organization_token_test.go
results:resource_tfe_team_token_test.go
results: