Skip to content
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

feat: Datetime constraint #1602

Merged
merged 33 commits into from
May 22, 2023
Merged

feat: Datetime constraint #1602

merged 33 commits into from
May 22, 2023

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented May 9, 2023 •
edited
Loading

Fixes: FLI-206

Adds new DateTime constraint type + UI

  • Allows specifying a Date and optional Time for a constraint value to match
  • Values accepted are either time.RFC3339 ("2006-01-02T15:04:05Z07:00") or time.Date ("2006-01-02")
  • UI picker requires at least a date be specified
  • Also adds ability to set how user wants times to be displayed (UTC or local) via new settings / preferences screen

Screens

DateTime Constraint

CleanShot 2023-05-09 at 16 49 00@2x CleanShot 2023-05-09 at 16 49 06@2x CleanShot 2023-05-17 at 21 31 36@2x

Preferences / Display

CleanShot 2023-05-17 at 21 26 56@2x CleanShot 2023-05-17 at 21 26 06@2x

TODO

  • more tests server side
  • UI Tests
  • add link (and docs PR) with link about DateTime constraint values explaining the format
  • potentially make the display timestamp nicer? not sure how though

GeorgeMac
GeorgeMac approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Change looks good from my end.

One thing to call out is that we should do the release dance of rpc/flipt and then sdk/go once that is bumped.

@markphelps
Merge branch 'main' into datetime-constraint-redo
7f57460 
* main:
  chore(deps): bump alpine from 3.17.3 to 3.18.0 in /build (#1603)
  chore: increasing test coverage to 80%
@codecov
Copy link

codecov bot commented May 10, 2023 •
edited
Loading

Codecov Report

Merging #1602 (d8a02eb) into main (c36160c) will increase coverage by 0.15%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##             main    #1602      +/-   ##
==========================================
+ Coverage   81.08%   81.23%   +0.15%     
==========================================
  Files          47       47              
  Lines        3786     3827      +41     
==========================================
+ Hits         3070     3109      +39     
- Misses        550      552       +2     
  Partials      166      166
Impacted Files Coverage Δ
internal/server/evaluator.go 94.89% <95.12%> (+0.02%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@markphelps
chore: change to store utc
8fc328d
@markphelps
feat(wip): timezone picker
32a3ed3
@markphelps
chore: timezone hook/provider
b1f3c94
@markphelps
chore: omg timezone
4a7678a
@markphelps
chore: merge main
37591b8
@markphelps
chore: drop support for Go 1.19 in tests
1d7fb8e
@markphelps
feat: different approach with timezone locales
aab2413
@markphelps
chore: forgot console.log; some cleanup
d5c6cd4
@markphelps
chore: more timezone fun
234a947
@markphelps
Merge branch 'main' into datetime-constraint-redo
360fd92
@markphelps
chore: make segment use inTimezone for display
94805f4
@markphelps
Merge branch 'main' into datetime-constraint-redo
d2c062e
@markphelps
fix: link to timezone in constraint form
2823f55
@markphelps
Merge branch 'datetime-constraint-redo' of https://github.com/flipt-i…
d1c2bc0 
…o/flipt into datetime-constraint-redo

* 'datetime-constraint-redo' of https://github.com/flipt-io/flipt:
  chore(deps): update all otel metrics related packages (#1628)
  chore(deps-dev): bump vite from 4.3.5 to 4.3.7 in /ui (#1627)
  chore(deps-dev): bump @types/node from 18.16.10 to 18.16.12 in /ui (#1626)
@markphelps
chore: move to preferences provider
d1079a9
@markphelps
chore: allow inputing datetimes according to prefs
c94ed23
@markphelps
fix: timezonez?
36c353d
@markphelps
chore: add a couple more tests for validation server side
6d39d14
@markphelps
Merge branch 'main' into datetime-constraint-redo
d75c856
@markphelps
fix: output reporting for ui ITs when running in CI
77fd62f
@markphelps
Merge branch 'datetime-constraint-redo' of https://github.com/flipt-i…
b44de61 
…o/flipt into datetime-constraint-redo

* 'datetime-constraint-redo' of https://github.com/flipt-io/flipt:
  chore(deps): bump react-router-dom from 6.11.1 to 6.11.2 in /ui (#1629)
@markphelps
fix: set CI in IT containers; try to use GH reporter in CI
76a3487
@markphelps markphelps marked this pull request as ready for review May 19, 2023 15:59
@markphelps markphelps requested a review from a team as a code owner May 19, 2023 15:59
@markphelps markphelps requested a review from GeorgeMac May 19, 2023 16:01
@markphelps
chore: rm uploading of playwright report
e924adf
@markphelps
Merge branch 'datetime-constraint-redo' of https://github.com/flipt-i…
d8a02eb 
…o/flipt into datetime-constraint-redo

* 'datetime-constraint-redo' of https://github.com/flipt-io/flipt:
  chore(deps-dev): bump @types/node from 18.16.12 to 18.16.13 in /ui (#1630)
  chore(deps-dev): bump vite from 4.3.7 to 4.3.8 in /ui (#1631)
GeorgeMac
GeorgeMac approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Very sweet 👌 Couple tiny cleanup things. Otherwise, this looks great.

.github/workflows/integration-test.yml Show resolved Hide resolved
internal/server/evaluator.go Outdated
return false, err
}

// TODO: we should consider parsing this at creation time since it doesn't change and it doesnt make sense to allow invalid constraint values
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can drop this comment now?

Suggested change
// TODO: we should consider parsing this at creation time since it doesn't change and it doesnt make sense to allow invalid constraint values

rpc/flipt/validation.go
// we know that a value is set and that the type is datetime
// so validate that the value is a valid datetime
// also convert it to UTC before we save
// TODO: don't love that we are doing this here
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 in the future maybe we move this next to the service/server layer.

yquansah
yquansah approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@yquansah yquansah left a comment

Choose a reason for hiding this comment

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

Nice one

@markphelps markphelps merged commit 124d8be into main May 22, 2023
@markphelps markphelps deleted the datetime-constraint-redo branch May 22, 2023 13:26
markphelps added a commit that referenced this pull request May 22, 2023
@markphelps
Merge branch 'main' into simpler-dev
6779477 
* main:
  chore(deps): bump go.opentelemetry.io/otel/exporters/jaeger (#1640)
  chore(deps-dev): bump eslint from 8.40.0 to 8.41.0 in /ui (#1634)
  chore(deps-dev): bump @playwright/test from 1.33.0 to 1.34.0 in /ui (#1632)
  chore(deps): bump github.com/cloudflare/circl in /build (#1607)
  chore: ignore playwright-report not existing (#1643)
  chore: fix ui lint issue with constraint form
  feat: Datetime constraint (#1602)
  chore(deps): bump github.com/lib/pq from 1.10.8 to 1.10.9 (#1638)
  chore(deps): bump github.com/coreos/go-oidc/v3 from 3.5.0 to 3.6.0 (#1639)
  chore(deps): bump golang.org/x/sync from 0.1.0 to 0.2.0 (#1641)
  chore(deps): bump golang.org/x/tools from 0.8.0 to 0.9.1 in /_tools (#1616)
  chore(deps): bump codecov/codecov-action from 3.1.3 to 3.1.4 (#1636)
  chore(deps-dev): bump @types/node from 18.16.13 to 18.16.14 in /ui (#1633)
  chore(deps-dev): bump @typescript-eslint/parser in /ui (#1620)
  chore(deps-dev): bump @types/node from 18.16.12 to 18.16.13 in /ui (#1630)
  chore(deps-dev): bump vite from 4.3.7 to 4.3.8 in /ui (#1631)
  chore(deps): bump react-router-dom from 6.11.1 to 6.11.2 in /ui (#1629)
  chore(deps): update all otel metrics related packages (#1628)
  chore(deps-dev): bump vite from 4.3.5 to 4.3.7 in /ui (#1627)
  chore(deps-dev): bump @types/node from 18.16.10 to 18.16.12 in /ui (#1626)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeMac GeorgeMac GeorgeMac approved these changes

@yquansah yquansah yquansah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markphelps @GeorgeMac @yquansah