Skip to content

More human friendly token expiry date messages#498

Merged
aaperis merged 6 commits intomainfrom
feature/token-expiry-date
Jan 31, 2025
Merged

More human friendly token expiry date messages#498
aaperis merged 6 commits intomainfrom
feature/token-expiry-date

Conversation

@aaperis
Copy link
Copy Markdown
Contributor

@aaperis aaperis commented Jan 23, 2025

Related issue(s) and PR(s)
This PR closes #437. Now token expiry messages to the user follow the output pattern that is listed in the issue description.

How to test
Tests have been added to the go testsuite, so

go test ./...

should be enough.

Note: The download integration tests fail because of the latest change where sda-download serves only /s3 whereas sda-cli looks for s3-encrypted. This is irrelevant to this PR though (see temporary fix #501)

@aaperis aaperis self-assigned this Jan 23, 2025
@aaperis aaperis requested a review from a team January 23, 2025 13:46
@aaperis aaperis force-pushed the feature/token-expiry-date branch from b5e0f5e to cb8bd2f Compare January 23, 2025 13:48
Copy link
Copy Markdown
Contributor

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

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

While you ar at it consider changing the default response of the switch to handle the case of an expired token.
The logical cases would then be:
1-120 min, 2-24 hours and > 24 hours.

Comment thread helpers/helpers.go Outdated
Comment thread helpers/helpers.go Outdated
Comment thread helpers/helpers.go Outdated
@aaperis
Copy link
Copy Markdown
Contributor Author

aaperis commented Jan 24, 2025

While you ar at it consider changing the default response of the switch to handle the case of an expired token. The logical cases would then be: 1-120 min, 2-24 hours and > 24 hours.

Makes, sense. Done.

@aaperis aaperis force-pushed the feature/token-expiry-date branch from abc7dde to 4874d03 Compare January 24, 2025 14:00
@aaperis aaperis requested review from a team and jbygdell January 24, 2025 14:01
Comment thread helpers/helpers.go Outdated
Copy link
Copy Markdown
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Looks great! I l left a minor comment for improvement but I'm OK to ignore it as well.

@aaperis aaperis force-pushed the feature/token-expiry-date branch from 4874d03 to 3fad701 Compare January 30, 2025 18:59
@aaperis aaperis requested review from a team and nanjiangshu January 30, 2025 19:23
Comment thread helpers/helpers.go Outdated
@aaperis aaperis requested a review from jbygdell January 31, 2025 07:58
@aaperis aaperis added this pull request to the merge queue Jan 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2025
@aaperis aaperis added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit 936c2c7 Jan 31, 2025
@aaperis aaperis deleted the feature/token-expiry-date branch January 31, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[session] Show token expiry time in a more Human friendly format

3 participants