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.

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

FS-1234; Expand DB Schema to allow storing Config Sets #43

Merged
merged 17 commits into from
May 7, 2024
Merged

Conversation

jakeschuurmans
Copy link
Contributor

@jakeschuurmans jakeschuurmans commented Apr 25, 2024

Adding Config Sets, unit tests, integration tests, mock tests, and the works to fleetdb.

There is more to talk about than can just be mentioned here in a description . . .

Main thing you might notice that might be alarming is I moved and renamed some of the "Attribute Operators". They have been moved to their own file "query_operators.go" so they can easily be shared.

I am also using urlquery to parse queries into structs and back. We can have a conversation about what we want to use for this purpose. This is the one that worked mostly well for my purpose.

Joel has requested that ConfigSets be renamed to BIOSConfigSets. I will make this change and put it together as its own commit

@jakeschuurmans jakeschuurmans changed the title FS-1234; 99% done. Still need to fix Update test FS-1234; Expand DB Schema to allow storing Config Sets Apr 30, 2024
@jakeschuurmans jakeschuurmans marked this pull request as ready for review April 30, 2024 14:56
@@ -21,7 +21,7 @@ jobs:
go-version: '1.22.1'

- name: Install cockroach binary
run: curl https://binaries.cockroachdb.com/cockroach-v23.1.11.linux-amd64.tgz | tar -xz && sudo cp -i cockroach-v23.1.11.linux-amd64/cockroach /usr/local/bin/
run: curl https://binaries.cockroachdb.com/cockroach-v21.1.21.linux-amd64.tgz | tar -xz && sudo cp -i cockroach-v21.1.21.linux-amd64/cockroach /usr/local/bin/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getting the CRDB error because the deprecated "crdb drop" command is no longer deprecated, and is now gone. The SQLBoiler version we use still uses drop.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like us to roll forward with versions and not use older ones in testing.

When you say SQLBoiler, I guess you're referring to the CrDB driver, specifically this https://github.com/metal-toolbox/sqlboiler-crdb/blob/18c5774bdcce987f22ef5ea1b542c2e390faae78/driver/override/templates_test/singleton/crdb_main_test.go.tpl#L56

Can you take a look if that can be updated - as a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a ticket for it. FS-1400

Copy link
Member

@joelrebel joelrebel left a comment

Choose a reason for hiding this comment

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

Nice work here, thanks for improving on the query handling.

Going ahead I'd like us to consider https://github.com/cbrand/go-filterparams since its fairly comprehensive.

Left a few comments and suggestions.

@@ -21,7 +21,7 @@ jobs:
go-version: '1.22.1'

- name: Install cockroach binary
run: curl https://binaries.cockroachdb.com/cockroach-v23.1.11.linux-amd64.tgz | tar -xz && sudo cp -i cockroach-v23.1.11.linux-amd64/cockroach /usr/local/bin/
run: curl https://binaries.cockroachdb.com/cockroach-v21.1.21.linux-amd64.tgz | tar -xz && sudo cp -i cockroach-v21.1.21.linux-amd64/cockroach /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

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

I'd like us to roll forward with versions and not use older ones in testing.

When you say SQLBoiler, I guess you're referring to the CrDB driver, specifically this https://github.com/metal-toolbox/sqlboiler-crdb/blob/18c5774bdcce987f22ef5ea1b542c2e390faae78/driver/override/templates_test/singleton/crdb_main_test.go.tpl#L56

Can you take a look if that can be updated - as a separate issue.

Makefile Show resolved Hide resolved
db/migrations/00002_config_sets.sql Outdated Show resolved Hide resolved
db/migrations/00002_config_sets.sql Outdated Show resolved Hide resolved
db/migrations/00002_config_sets.sql Outdated Show resolved Hide resolved
pkg/api/v1/router.go Outdated Show resolved Hide resolved
pkg/api/v1/router_config_set.go Outdated Show resolved Hide resolved
pkg/api/v1/router_config_set.go Outdated Show resolved Hide resolved
pkg/api/v1/router_config_set.go Outdated Show resolved Hide resolved
pkg/api/v1/router_config_set.go Outdated Show resolved Hide resolved
joelrebel
joelrebel previously approved these changes May 7, 2024
Copy link
Member

@joelrebel joelrebel left a comment

Choose a reason for hiding this comment

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

thanks, lets get this merged 🚀

@jakeschuurmans jakeschuurmans merged commit cd8682b into main May 7, 2024
1 check passed
@jakeschuurmans jakeschuurmans deleted the FS-1234 branch May 7, 2024 13:39
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.

2 participants