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: add server configuration #39

Merged
merged 6 commits into from
Nov 19, 2024
Merged

feat: add server configuration #39

merged 6 commits into from
Nov 19, 2024

Conversation

markphelps
Copy link
Contributor

@markphelps markphelps commented Nov 18, 2024

Fixes: FLI-1338

  • Make server configurable with addr, port, protocol, etc
  • Make setDefaults and validate run automatically for types that implement them in the config struct
  • Standardize some error messages
  • Added some configuration documentation for the docs site
workspace/glu - [api-config●] » GLU_SERVER_PORT=8081 go run ./examples/fakedata/...
2024/11/18 14:29:48 WARN could not locate glu.yaml
time=2024-11-18T14:29:48.326-05:00 level=INFO msg="starting server" addr=0.0.0.0:8081

@markphelps markphelps requested a review from GeorgeMac November 18, 2024 19:32
Copy link
Member

@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.

Nice. Have some feedback. This is currently breaking defaults / validation for Credentials and Sources (we must be nearing test time 😂).

For example, when running this code with our internal pipeline. I see the following error when I use basic auth without a password:

➜  go run ./cmd/glu/. inspect
panic: building pipeline: git "foundation": performing initial fetch: invalid auth method

With either of the suggestions below this becomes:

➜  go run ./cmd/glu/. inspect
panic: configuring pipeline: both username and password need to be provided for basic auth

pkg/config/oci.go Outdated Show resolved Hide resolved
pkg/config/git.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
@markphelps markphelps requested a review from GeorgeMac November 19, 2024 15:58
@markphelps
Copy link
Contributor Author

@GeorgeMac I think I addressed all feedback and even added some basic unit tests for the processValue func

Copy link
Member

@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.

Sick

docs/configuration.md Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit 02219bb into main Nov 19, 2024
2 checks passed
@kodiakhq kodiakhq bot deleted the api-config branch November 19, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants