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

feat(cat-gateway): config API #981

Merged
merged 47 commits into from
Oct 18, 2024
Merged

feat(cat-gateway): config API #981

merged 47 commits into from
Oct 18, 2024

Conversation

bkioshn
Copy link
Contributor

@bkioshn bkioshn commented Oct 9, 2024

Description

Implement a config API endpoint

Related Issue(s)

Closes #926

Description of Changes

Create a config endpoint
GET /api/draft/config/frontend

  • Get IP from X-Real-IP, Forwarded, X-Forwarded-For or Remote Address.

GET /api/draft/config/frontend/schema

  • Get the frontend JSON schema

PUT /api/draft/config/frontend?ip=<ipAddress>

  • Set the config for the specify IP if it already exist in the table, if not exist insert a new row
  • If ip is not specify, update the general config

Additional Changes

  • The value for JSON is validated with JSON schema.
  • Default value is implemented.
  • Remove refinery_schema_history v2 to v9

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Test Report | ${\color{lightgreen}Pass: 297/297}$ | ${\color{red}Fail: 0/297}$ |

Signed-off-by: bkioshn <[email protected]>
@bkioshn bkioshn self-assigned this Oct 9, 2024
@bkioshn bkioshn added the enhancement New feature or request label Oct 9, 2024
@bkioshn bkioshn added this to the M4: Voting & Delegation milestone Oct 9, 2024
@stevenj
Copy link
Collaborator

stevenj commented Oct 9, 2024

Re endpoints. Use:

GET /api/draft/config/frontend
PUT /api/draft/config/frontend?ip=<ip4addr/ip6addr>

GET and PUT are enough to signify if it's a read or write.

PUT is more correct because we are setting the whole resource, its "idempotent". Doing the same PUT multiple times is the same as doing it once.

There is no requirement for ip as a parameter on GET. It should be obtained directly from the headers of the request. See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For Poem should have a way to get this. There are other headers which could also carry the clients IP address. The logic should be, Get the general config, and the IP config, replace or set any key in the general config with the keys from the ip config (merge the json).

On PUT, the ip addr is a query parameter, if its specified you write the IP config record with the JSON given (if its valid) and it its not, you write the general config.

Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

See comments for requested changes

catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/event-db/migrations/V1__config_tables.sql Outdated Show resolved Hide resolved
@bkioshn bkioshn requested a review from stevenj October 10, 2024 07:49
@bkioshn bkioshn added the review me PR is ready for review label Oct 10, 2024
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/key.rs Outdated Show resolved Hide resolved
Mr-Leshiy
Mr-Leshiy previously approved these changes Oct 14, 2024
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

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

LGTM

Mr-Leshiy
Mr-Leshiy previously approved these changes Oct 16, 2024
catalyst-gateway/bin/src/db/event/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/event/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/service/api/config/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenj stevenj merged commit 63fee26 into main Oct 18, 2024
37 checks passed
@stevenj stevenj deleted the feat/config-api branch October 18, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🛠️ [TASK] : Create general configuration endpoint
3 participants