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 include: feature #1008

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

imjasonh
Copy link
Member

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.

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>
@lpcalisi
Copy link

lpcalisi commented Jan 12, 2024

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

@imjasonh
Copy link
Member Author

@imjasonh I was thinking about that include feature to incorporate some base configurations to all apko definitions, like inheritance. Users, nvironments 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.

@lpcalisi
Copy link

lpcalisi commented Jan 12, 2024

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

@imjasonh
Copy link
Member Author

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?

@lpcalisi
Copy link

@imjasonh i made the changes, could you take a look?

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.

2 participants