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: basic Caddyfile support + tests #6

Merged
merged 4 commits into from
Oct 16, 2020

Conversation

dunglas
Copy link
Collaborator

@dunglas dunglas commented Oct 6, 2020

No description provided.

@dunglas dunglas force-pushed the feat/caddyfile branch 2 times, most recently from e55a048 to b6bd2d3 Compare October 7, 2020 06:59
@dunglas dunglas marked this pull request as ready for review October 7, 2020 07:00
httpcache.go Outdated
// cache {
// default_ttl <ttl>
// max_size <size>
// olric_env <env>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

olric_env foo

or

olric {
   env foo
}

What's the usual convention @mholt @francislavoie?

Copy link
Member

Choose a reason for hiding this comment

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

Are there other olric options to go in? If so, grouping it might make sense, but otherwise, why not just env?

Depends if we want to "leak" that olric is the underlying implementation. Does that matter to the user? We'd need to explain what "olric" is for them to know what that's intended for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there will be more to support the clustered mode (cc @buraksezer).

Copy link
Member

Choose a reason for hiding this comment

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

My impulse is to prefer env foo as a subdirective. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Olric has many configuration directives. Mostly key/value but it can also contain lists and maps. I don't know how we translate it into Caddyfile format. Here is a sample for olricd (includes service discovery sections for Consul): https://github.com/buraksezer/olric/blob/master/docker/olricd-consul.yaml

If it's complicated to build or maintain, we may read the yaml file from the disk and translate into the configuration for embedded mode. olricd is already doing this job.

cache {
    default_ttl 5
    olric_config /path/to/olric.yaml
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!!

Copy link
Contributor

Choose a reason for hiding this comment

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

@dunglas config.Load function is ready to use. See v0.3.0-beta.6

Simply:

c, err := config.Load(cpath)
if err != nil {
// handle this error
}

Copy link
Member

Choose a reason for hiding this comment

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

(btw @buraksezer I think you have the first two issues in that changelog flipped)

Copy link
Contributor

Choose a reason for hiding this comment

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

@francislavoie Thanks. Fixed!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Support added in 9a0d86c!

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Beautiful. Great start for Caddyfile support.

httpcache.go Outdated
// cache {
// default_ttl <ttl>
// max_size <size>
// olric_env <env>
Copy link
Member

Choose a reason for hiding this comment

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

My impulse is to prefer env foo as a subdirective. 👍

@dunglas
Copy link
Collaborator Author

dunglas commented Oct 9, 2020

I added a commit with an alternative that implementation that uses a global option. It makes it explicit that the cache configuration is global and cannot be modified:

{
    # optional, but the cache can be configured only here, not in a specific handler
    cache {
        olric_config /path/to/olric.yaml
    }
}

localhost

route /cache {
    cache
    header Cache-Control "s-maxage=5"
    respond "Hello!"
}

route /other-cache {
    cache
    header Cache-Control "s-maxage=60"
    respond "Hello, World!"
}

This format is (IMHO) less error-prone and more intuitive.

In JSON, the configuration of the cache must be exactly the same for every handler or a validation error is thrown.

I also followed @buraksezer's suggestion to simply allow users to configure Olric using its own YAML format, it will simplify the maintenance a lot: users will automatically have access to all Olric's features, even new ones.

WDYT @mholt @francislavoie?

@mholt
Copy link
Member

mholt commented Oct 9, 2020

@dunglas I think moving the cache config to a global option is definitely a good idea, looks great.

It should still need to be documented that even the singular/global cache config cannot change even though config reloads, as users will naturally expect that if they change their config and reload it that it will update the cache's config too.

@dunglas
Copy link
Collaborator Author

dunglas commented Oct 9, 2020

On my side, this PR is ready to be merged!

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Think I'm good with this too. Solid start!

@mholt
Copy link
Member

mholt commented Oct 15, 2020

(PS. You may merge this whenever you're ready!)

@dunglas
Copy link
Collaborator Author

dunglas commented Oct 16, 2020

@mholt This PR is ready to be merged, but I haven't the rights to do so.

@mholt
Copy link
Member

mholt commented Oct 16, 2020

Oops! My bad. I just added you as a maintainer.

@dunglas dunglas merged commit 8c7b791 into caddyserver:master Oct 16, 2020
@dunglas dunglas deleted the feat/caddyfile branch October 16, 2020 19:49
@dunglas
Copy link
Collaborator Author

dunglas commented Oct 17, 2020

Thanks @mholt!

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.

4 participants