-
Notifications
You must be signed in to change notification settings - Fork 131
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 include: feature #1008
base: main
Are you sure you want to change the base?
remove include: feature #1008
Conversation
Signed-off-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Jason Hall <jason@chainguard.dev>
@imjasonh I was thinking about that include feature to incorporate some base configurations to all apko definitions, like inheritance. Users, environments variables, paths, or packages for tooling in some cases. |
That seems reasonable. I would still like to remove remote includes, since that seems more likely to cause bad bugs than helpful experiences. I'd also like to invest in better (any) tests for this feature if anyone is actually using it. There might be confusing corner cases involved in merging two configs, that we should at least document in a test. If you're interested in adding those tests or using that behavior, let me know. |
i could add some tests and improve documentation about how use this feature, if it still make sense for you. A couple a weeks ago i made this #996 in order to extend definitions of include, i could try to work in this PR to add tests |
Sorry I missed that PR. It looks good so far, more tests are always welcome. Sounds like we should keep local includes, but not remote. Does that sound good? |
@imjasonh i made the changes, could you take a look? |
Opening for discussion.
We don't use this feature, and I don't know of anybody who does or would. It presents a potentially confusing or abuse-prone surface.