Skip to content

Add max duration to msteams plugin cards#54110

Merged
EdwardDowling merged 13 commits intomasterfrom
edwarddowling/msteams-request-duration
Apr 29, 2025
Merged

Add max duration to msteams plugin cards#54110
EdwardDowling merged 13 commits intomasterfrom
edwarddowling/msteams-request-duration

Conversation

@EdwardDowling
Copy link
Copy Markdown
Contributor

@EdwardDowling EdwardDowling commented Apr 17, 2025

Part of #43555

Adds max duration to the msteam plugin's cards

changelog: Include access request's max duration in MsTeams plugin messages

@EdwardDowling EdwardDowling force-pushed the edwarddowling/msteams-request-duration branch from 21098e6 to 341a4ef Compare April 23, 2025 14:41
@EdwardDowling EdwardDowling marked this pull request as ready for review April 23, 2025 14:41
Resources []string
SuggestedReviewers []string
LoginsByRole map[string][]string
MaxDuration *time.Time
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by the type here. The name says "duration", but this is a "time" object. Should it be the access request end date, or a time.Duration?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the the // GetMaxDuration gets the maximum time at which the access should be approved for. I think we just call it maxDuration since we take it in as a duration usually and convert it to the target time on request creation to avoid having to store the duration and the time the duration is from.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I never realized that we made this mistake 2 years ago when introducing max duration 🤦 .
MaxDuration on a role is indeed a duration, but because naming is hard we also named it MaxDuration on the access request even though it's a timestamp.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we did this error, can we at least rename this field to be LatestApprovalTime?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would changing the name only here make things clearer or would that just add to the existing confusion?

@EdwardDowling
Copy link
Copy Markdown
Contributor Author

@bernardjkim @tigrato Could one of you take a look at this when you get a second?

Comment thread integrations/lib/plugindata/access_request.go
Resources []string
SuggestedReviewers []string
LoginsByRole map[string][]string
MaxDuration *time.Time
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we did this error, can we at least rename this field to be LatestApprovalTime?

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from bernardjkim April 25, 2025 15:48
@EdwardDowling EdwardDowling enabled auto-merge April 29, 2025 15:55
@EdwardDowling EdwardDowling added this pull request to the merge queue Apr 29, 2025
Merged via the queue into master with commit aa0e35f Apr 29, 2025
40 checks passed
@EdwardDowling EdwardDowling deleted the edwarddowling/msteams-request-duration branch April 29, 2025 16:35
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@EdwardDowling See the table below for backport results.

Branch Result
branch/v16 Create PR
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants