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

Git resolver #99

Closed
wants to merge 2 commits into from
Closed

Git resolver #99

wants to merge 2 commits into from

Conversation

davinci26
Copy link
Collaborator

Fixes #98

This adds two new command line flags:

  • -git: Which controls if paths are treated as versioned paths
  • -repo: Which set the path to the git repository

Now all include paths are parsed to extract the version (git version) if present. If present we resolve the file with git show rather than opening the file directly

chose to use git cmd rather than an in memory git implementation because it is harder with authentication etc

Resolve vcl includes based on a set of versioned directories.

Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
// if (req.http.Host) {
// set req.http.Fourth = re.group.3; // difficult to know which (1) or (2) matched result is used or empty
// }
// if (req.http.Host) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

linter is not happy without this change 🤷

@ysugimoto
Copy link
Owner

I have a couple of opinions.

  1. How about switching the resolver from the target file starting with git://github.com ?

Example:

falco -v git://github.com/org/repo # use GIt resolver
falco -v ./main.vcl # use File resolver
  1. Why don't we use https://github.com/google/go-github ?

I understand that the git command is useful for resolving authentication like private repo, but it means that falco has a dependency on git command. Normally git command has been installed on almost the developer's machine but we should discuss it.

@davinci26
Copy link
Collaborator Author

Why don't we use https://github.com/google/go-github ?

I started implementing this with https://github.com/go-git/go-git but I thought in the end that it wasnt worth it. Mainly for two reasons:

  • Authentication is complicated and it will require us to extend the command line with even more flags.
  • We will add a big golang dependency that is used for only a single command

Basically at this point given that we do only one git call it seems like a decent tradeoff not to pull an entire git implementation and rely on a subprocess call. Especially because I didnt have to handle the output and I just take the whole stdout of the process.

How about switching the resolver from the target file starting with git://github.com ?

Does this mean adding the prefix git://github.com for all paths? Or just for the main.vcl. That prefix might not be a bad idea since it is used widely

@ysugimoto
Copy link
Owner

ysugimoto commented Aug 29, 2022

Does this mean adding the prefix git://github.com for all paths? Or just for the main.vcl. That prefix might not be a bad idea since it is used widely

Yes, for example, if git://github.com/org/repo is provided, it adds to include path and resolves include statements from another git repository via git show command.

And from another perspective, I think the following commands are the same behavior for managing VCLs on another repository:

git clone [email protected]:org/repo # ssh or https
falco -I ./repo path/to/main.vcl

The above syntax is the common use case and clearly divides the matter of concern git authentication I think, so I'm not sure that the worth of supporting git communication (of course git submodule is not a good way, but...)

It is useful for the continuous integration of local development, Or if there are more benefits, please let me know.

@davinci26
Copy link
Collaborator Author

git clone [email protected]:org/repo # ssh or https
falco -I ./repo path/to/main.vcl

Sadly this is not exactly the same and this is the problem I run into:

Imagine that you have repo 1 that contains:

  • main_1.vcl
  • main_2vcl

Then main_1.vcl could depend on version 1 of a dependency and main_2.vcl on version 2. And even within just main_1.vcl it could have a dependency that is on version 1 and another that is on version 2 so you can checkout the repo, you have to show each dependency on the correct version

@ysugimoto ysugimoto added the enhancement New feature or request label Aug 30, 2022
@ysugimoto
Copy link
Owner

@davinci26 Hmm, could you show me in detail of use case using github repo?

  • What does the version mean? fastly's service version?
  • Is there a case of managing VCL across versions?

I could not imagine that some VCL file depends on other versions. In my use case, all VCL files indicate one version and it's the standard way of managing VCL.

@shadialtarsha @ivomurrell could you hear me your opinition on this case?

@ivomurrell
Copy link
Collaborator

If I understand correctly than some special tooling is required when deploying VCL that specifies Git versions for dependencies, as Fastly's include statement does not support specifying revisions itself. Unless this format of resolving files via a path:version string is standardised and used across various organisations I'm not sure whether we should be supporting it in the linter. Is this something that needs to be upstreamed, @davinci26?

@davinci26
Copy link
Collaborator Author

@ivomurrell I think that it is a good idea to use standard git uri for include paths which make it more upstream friendly and also I think versioned VCL files is a natural progression if you manage a big VCL corpus so this is why I wanted to push it.

Imagine that you have the following scenario:

  • helpers.vcl in common repository
  • service1.vcl and service2.vcl in a single repository
  • service1 gets way way more traffic than service2

Then you want to rollout the helpers.vcl in service2 first for stability reasons and then you roll it our on service1 after you validated first. In that case you have the following:

  • time 0: Both service 1 and service 2 are on version a
  • time 1: service 2 is updated on version b
  • time 2: service 1 is also updated to version b

@ivomurrell
Copy link
Collaborator

@davinci26 I can definitely see the utility of the approach you're describing, but the problem still seems to be that the rollout facility depends on custom deployment tooling to parse and resolve the special path:version syntax. I'm not convinced it makes sense to put this in the upstream falco repository rather than keep as a fork you can maintain when we're talking about adding support for a bespoke solution specific to your company 🙂 Is the code you use to resolve the git revisions when making deployments to Fastly available to look at somewhere?

@davinci26
Copy link
Collaborator Author

Realized that the TF mode is solving all my problem and I can TF plan to do this for me. I think this is a win-win overall :)

This is no longer needed, thanks a lot for the feedback tho!

@davinci26
Copy link
Collaborator Author

See #102

@davinci26 davinci26 closed this Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Git Based Resolver
3 participants