Skip to content

chore: Make deployment admin page show better durations #6856

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

Merged
merged 7 commits into from
Mar 29, 2023
Merged

chore: Make deployment admin page show better durations #6856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be736f2
chore: Make deployment admin page show better durations
Emyrk Mar 28, 2023
8a20218
Linting
Emyrk Mar 28, 2023
44160dd
make gen
Emyrk Mar 28, 2023
9177fea
Remove 0 as unset
Emyrk Mar 29, 2023
933539e
Add unit test for optionValue
Emyrk Mar 29, 2023
0b3f76e
update golden
Emyrk Mar 29, 2023
f4a0826
make fmt
Emyrk Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions cli/clibase/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,19 +190,6 @@ func (d *Duration) String() string {
return time.Duration(*d).String()
}

func (d *Duration) MarshalJSON() ([]byte, error) {
return json.Marshal(d.String())
}

func (d *Duration) UnmarshalJSON(b []byte) error {
var s string
err := json.Unmarshal(b, &s)
if err != nil {
return err
}
return d.Set(s)
}

func (Duration) Type() string {
return "duration"
}
Expand Down
2 changes: 1 addition & 1 deletion cli/testdata/coder_server_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ backed by Tailscale and WireGuard.
--http-address string, $CODER_HTTP_ADDRESS (default: 127.0.0.1:3000)
HTTP bind address of the server. Unset to disable the HTTP endpoint.

--max-token-lifetime duration, $CODER_MAX_TOKEN_LIFETIME (default: 2562047h47m16.854775807s)
--max-token-lifetime duration, $CODER_MAX_TOKEN_LIFETIME (default: 876600h0m0s)
The maximum lifetime duration users can specify when creating an API
token.

Expand Down
12 changes: 1 addition & 11 deletions coderd/apikey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"database/sql"
"errors"
"fmt"
"math"
"net"
"net/http"
"strconv"
Expand Down Expand Up @@ -354,19 +353,10 @@ func (api *API) tokenConfig(rw http.ResponseWriter, r *http.Request) {
return
}

var maxTokenLifetime time.Duration
// if --max-token-lifetime is unset (default value is math.MaxInt64)
// send back a falsy value
if values.MaxTokenLifetime.Value() == time.Duration(math.MaxInt64) {
maxTokenLifetime = 0
} else {
maxTokenLifetime = values.MaxTokenLifetime.Value()
}

httpapi.Write(
r.Context(), rw, http.StatusOK,
codersdk.TokenConfig{
MaxTokenLifetime: maxTokenLifetime,
MaxTokenLifetime: values.MaxTokenLifetime.Value(),
},
)
}
Expand Down
12 changes: 7 additions & 5 deletions codersdk/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"flag"
"math"
"net/http"
"os"
"path/filepath"
Expand Down Expand Up @@ -1113,10 +1112,13 @@ when required by your organization's security policy.`,
Description: "The maximum lifetime duration users can specify when creating an API token.",
Flag: "max-token-lifetime",
Env: "CODER_MAX_TOKEN_LIFETIME",
Default: time.Duration(math.MaxInt64).String(),
Value: &c.MaxTokenLifetime,
Group: &deploymentGroupNetworkingHTTP,
YAML: "maxTokenLifetime",
// The default value is essentially "forever", so just use 100 years.
// We have to add in the 25 leap days for the frontend to show the
// "100 years" correctly.
Default: ((100 * 365 * time.Hour * 24) + (25 * time.Hour * 24)).String(),
Value: &c.MaxTokenLifetime,
Copy link
Member

Choose a reason for hiding this comment

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

In apikey.go, we check to see if --max-token-lifetime is unset. I think we'll have to adjust this logic now that we aren't using time.Duration(math.MaxInt64).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof, I hate this maxint buisness. We should have just used 0 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We can bring back in "unset" if we need later.

Group: &deploymentGroupNetworkingHTTP,
YAML: "maxTokenLifetime",
},
{
Name: "Enable swagger endpoint",
Expand Down
2 changes: 1 addition & 1 deletion docs/cli/server.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ Output Stackdriver compatible logs to a given file.
| ----------- | -------------------------------------- |
| Type | <code>duration</code> |
| Environment | <code>$CODER_MAX_TOKEN_LIFETIME</code> |
| Default | <code>2562047h47m16.854775807s</code> |
| Default | <code>876600h0m0s</code> |

The maximum lifetime duration users can specify when creating an API token.

Expand Down
73 changes: 73 additions & 0 deletions site/src/components/DeploySettingsLayout/Options.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { optionValue } from "./OptionsTable"
import { DeploymentOption } from "api/types"

const defaultOption: DeploymentOption = {
name: "",
description: "",
flag: "",
flag_shorthand: "",
value: "",
hidden: false,
}

describe("optionValue", () => {
it.each<{
option: DeploymentOption
expected: string | string[] | unknown
}>([
{
option: {
...defaultOption,
name: "Max Token Lifetime",
value: 3600 * 1e9,
},
expected: "1 hour",
},
{
option: {
...defaultOption,
name: "Max Token Lifetime",
value: 24 * 3600 * 1e9,
},
expected: "1 day",
},
{
option: {
...defaultOption,
name: "Session Duration",
value: 3600 * 1e9,
},
expected: "1 hour",
},
{
option: {
...defaultOption,
name: "Session Duration",
value: 24 * 3600 * 1e9,
},
expected: "1 day",
},
{
option: {
...defaultOption,
name: "Strict-Transport-Security",
value: 1000,
},
expected: "1000s",
},
{
option: {
...defaultOption,
name: "OIDC Group Mapping",
value: {
"123": "foo",
"456": "bar",
"789": "baz",
},
},
expected: [`"123"->"foo"`, `"456"->"bar"`, `"789"->"baz"`],
},
])(`[$option.name]optionValue($option.value)`, ({ option, expected }) => {
expect(optionValue(option)).toEqual(expected)
})
})
34 changes: 32 additions & 2 deletions site/src/components/DeploySettingsLayout/OptionsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from "components/DeploySettingsLayout/Option"
import { FC } from "react"
import { DisabledBadge } from "./Badges"
import { intervalToDuration, formatDuration } from "date-fns"

const OptionsTable: FC<{
options: DeploymentOption[]
Expand All @@ -34,7 +35,11 @@ const OptionsTable: FC<{
</TableHead>
<TableBody>
{Object.values(options).map((option) => {
if (option.value === null || option.value === "") {
if (
option.value === null ||
option.value === "" ||
option.value === undefined
) {
return null
}
return (
Expand All @@ -45,7 +50,7 @@ const OptionsTable: FC<{
</TableCell>

<TableCell>
<OptionValue>{option.value}</OptionValue>
<OptionValue>{optionValue(option)}</OptionValue>
</TableCell>
</TableRow>
)
Expand All @@ -56,6 +61,31 @@ const OptionsTable: FC<{
)
}

// optionValue is a helper function to format the value of a specific deployment options
export function optionValue(
option: DeploymentOption,
): string[] | string | unknown {
switch (option.name) {
case "Max Token Lifetime":
Copy link
Member

Choose a reason for hiding this comment

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

What's up with this empty case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't case blocks fall through to each other? These two cases are handled the same.

Copy link
Member

Choose a reason for hiding this comment

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

:D I had to google that - you're right.

Down the road, I wonder if we should type DeploymentOption.flag as something other than string (like we should return an enum). Then we could have an object lookup on the FE, like:

const deploymentOptionFormats: { [key: FlagEnum]: (option: DeploymentOption) => string[] | string | unknown } = {
    flagName1: (option) => {
        // format and return
    },
    ....
}

// in the JSX
<OptionValue>{deploymentOptionFormats[option.flag](option)}</OptionValue>

This would give us some additional type safety. Not something to worry about in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Totally makes sense. I didn't know the "typescript" way to do this.

case "Session Duration":
// intervalToDuration takes ms, so convert nanoseconds to ms
return formatDuration(
intervalToDuration({ start: 0, end: (option.value as number) / 1e6 }),
)
case "Strict-Transport-Security":
if (option.value === 0) {
return "Disabled"
}
return (option.value as number).toString() + "s"
case "OIDC Group Mapping":
return Object.entries(option.value as Record<string, string>).map(
([key, value]) => `"${key}"->"${value}"`,
)
default:
return option.value
}
}

const useStyles = makeStyles((theme) => ({
table: {
"& td": {
Expand Down
4 changes: 0 additions & 4 deletions site/src/pages/CreateTokenPage/utils.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ describe("unit/CreateTokenForm", () => {
maxTokenLifetime: number
expected: LifetimeDay[]
}>([
{
maxTokenLifetime: 0,
expected: lifetimeDayPresets,
},
{ maxTokenLifetime: 6 * 24 * NANO_HOUR, expected: [] },
{
maxTokenLifetime: 20 * 24 * NANO_HOUR,
Expand Down