Skip to content

feat(licenses): show license_expires time as rfc3339 date #7687

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 26, 2023
Merged

feat(licenses): show license_expires time as rfc3339 date #7687

merged 2 commits into from
May 26, 2023

Conversation

rodrimaia
Copy link
Contributor

@rodrimaia rodrimaia commented May 25, 2023 •
edited
Loading

Show the license_expires field as a human-readable date, as discussed on #4313 .

  • Only editing the license_expires field, should we apply the same logic to other fields?
  • Used the RFC3339 format.

@matifali wdyt of receiving a new --format parameter to choose between table and json ?

image

Fixes #4313

@rodrimaia rodrimaia self-assigned this May 25, 2023
matifali
matifali reviewed May 26, 2023
View reviewed changes
Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

This resolved the original issue where the date was not human readable. Output format was an extra suggestion that IMO should default to table unless we explicitly request it to be in json with --format json

mtojek
mtojek requested changes May 26, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

This resolved the original issue where the date was not human readable. Output format was an extra suggestion that IMO should default to table unless we explicitly request it to be in json with --format json

It is tricky as in theory, it would be a breaking change. Not sure if something consumes the output, but maybe it is worth changing to a default table format (separate PR please).

BTW Mark also reported this problem:

It also seems like a security risk that a Salesforce identifier is exposed in this file.

@rodrimaia rodrimaia mentioned this pull request May 26, 2023
Closed
@rodrimaia
Copy link
Contributor Author

This resolved the original issue where the date was not human readable. Output format was an extra suggestion that IMO should default to table unless we explicitly request it to be in json with --format json

I've created a new issue to track it: #7697

@rodrimaia
Copy link
Contributor Author

This resolved the original issue where the date was not human readable. Output format was an extra suggestion that IMO should default to table unless we explicitly request it to be in json with --format json

It is tricky as in theory, it would be a breaking change. Not sure if something consumes the output, but maybe it is worth changing to a default table format (separate PR please).

BTW Mark also reported this problem:

It also seems like a security risk that a Salesforce identifier is exposed in this file.

@mtojek there is no security risk accordingly to this comment: #4313 (comment) :)

matifali
matifali approved these changes May 26, 2023
View reviewed changes
Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

The final result looks good to me.
You may address @mtojek comments on code.

@mtojek mtojek self-requested a review May 26, 2023 14:30
mtojek
mtojek approved these changes May 26, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

👍

@rodrimaia rodrimaia enabled auto-merge (squash) May 26, 2023 14:31
@rodrimaia rodrimaia merged commit 61dc875 into main May 26, 2023
@rodrimaia rodrimaia deleted the display_license_expiration_date branch May 26, 2023 14:36
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2023
@ammario
Copy link
Member

ammario commented May 30, 2023

We shouldn't worry about minor breaking changes right now, especially in interfaces that are unlikely to be automated around.

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

@matifali matifali matifali approved these changes

@mtojek mtojek mtojek approved these changes

@sharkymark sharkymark Awaiting requested review from sharkymark

Assignees

@rodrimaia rodrimaia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: coder license list shows a unix date and not a human-readable date
4 participants
@rodrimaia @ammario @matifali @mtojek