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

Remove config being picked up in the CLI #711

Closed
wants to merge 1 commit into from
Closed

Conversation

XAMPPRocky
Copy link
Collaborator

Part of #700 this PR removes Config from being picked up in the CLI for now. So now the only way to provide configuration is through the file provider for a service.

@quilkin-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 057cc317-f701-4177-a450-9ca38055faf8

Status: FAILURE

To get permission to view the Cloud Build view, join the quilkin-discuss Google Group.

Filter with the Git Commit cae0736 within us-docker.pkg.dev/quilkin/ci//quilkin to see if a development image is available from this build, and can be used for debugging purposes.

Development images are retained for at least 30 days.

@markmandel
Copy link
Contributor

Did you delete your comment?

Looks like the change broke the test that attempts to run the default Docker proxy image:

Already have image (with digest): gcr.io/cloud-builders/docker
{"timestamp":"2023-07-18T12:21:12.198881Z","level":"INFO","fields":{"message":"Starting Quilkin","version":"0.7.0-dev","commit":"cae0736c50862e687407775846eda1d351155ca4"},"target":"quilkin::cli","filename":"src/cli.rs"}
{"timestamp":"2023-07-18T12:21:12.199228Z","level":"INFO","fields":{"message":"Starting admin endpoint","address":"[::]:8000"},"target":"quilkin::admin","filename":"src/admin.rs"}
{"timestamp":"2023-07-18T12:21:12.199669Z","level":"ERROR","fields":{"message":"fatal error","error":"`quilkin proxy` requires at least one `to` address or `management_server` endpoint.","error_debug":"`quilkin proxy` requires at least one `to` address or `management_server` endpoint."},"target":"quilkin","filename":"src/main.rs"}

quilkin/cloudbuild.yaml

Lines 55 to 63 in ea5509f

- name: gcr.io/cloud-builders/docker
dir: ./build
entrypoint: bash
args:
- '-c'
- 'timeout --signal=INT --preserve-status 5s docker run --rm -v "/workspace/examples/proxy.yaml:/etc/quilkin/quilkin.yaml" ${_REPOSITORY}quilkin:$(make version) proxy'
id: test-quilkin-image-default-config-file
waitFor:
- build

Do we want to remove the Config search completely for this development path?

If we want to maintain a search path, and since we'll have multiple files we could do /etc/quilkin/quilkin.yaml (maybe for a backward compatibility / not break tests), and then load all yaml files in /etc/quilkin/quilkin.d/ for all the different Kind of configurations.

That would align with Linux config and we could likely keep a backward compatibility, merge features towards #700, still do releases and continue with this work until complete.

WDYT?

@XAMPPRocky
Copy link
Collaborator Author

Yeah I figured out what's wrong with this, I need to also add the file provider to the proxy service so that the examples work.

Do we want to remove the Config search completely for this development path?

No no, I want move proxy to being provider based like the other services. So you'd write quilkin proxy file instead, because that would also allow for quilkin proxy agones where you have the discovery happening in a single service.

@markmandel
Copy link
Contributor

No no, I want move proxy to being provider based like the other services. So you'd write quilkin proxy file instead, because that would also allow for quilkin proxy agones where you have the discovery happening in a single service.

Got it 👍🏻

Yeah, that makes 100% sense then to remove it.

Or if you want to do this + quilkin proxy file, all in one PR, that could mean we could keep the cloudbuild test step?

Just thinking through removing and adding test steps is all.

@XAMPPRocky
Copy link
Collaborator Author

XAMPPRocky commented Jul 18, 2023

Yeah exactly, I'll update this with adding that feature, we shouldn't disable the tests.

@XAMPPRocky
Copy link
Collaborator Author

Needs more to be mergeable

@XAMPPRocky XAMPPRocky closed this Nov 25, 2023
@markmandel markmandel deleted the ep/rm-config-cli branch March 11, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants