-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Elastic Agent] Fix issues with dynamic inputs and conditions #23886
[Elastic Agent] Fix issues with dynamic inputs and conditions #23886
Conversation
Pinging @elastic/agent (Team:Agent) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested ok with standalone config, code lgtm
looks like config from #23685 works
return newConfigFrom(c), err | ||
} | ||
|
||
c, err := ucfg.NewFrom(from, opts...) | ||
return newConfigFrom(c), err | ||
var c map[string]interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we generalize this a bit? e.g have a list of keywords where we want to skip ucfg resolution.
i think extraction to func and var list of keys would be enough
Pinging @elastic/ingest-management (Team:Ingest Management) |
/pacakge |
/package |
1 similar comment
/package |
@michalpristas I cleaned up the config loading more. See what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks ok. i like having load defines inside config package
} | ||
local := &options{} | ||
for _, o := range localOpts { | ||
o(local) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with multiple VarSkipKeys last one wins right? should we allow multiple entries by combining them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes last wins. If we combine them, then no way to overwrite.... idk its a choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm bringing this up just to verify it's a decision not a bug
Thank you for fixing this one @blakerouse . Could you provide an example of valid configuration based on your fixes (dynamic input + env vars), or point me to a docs page which includes such examples? It's still not clear to me what is the proper way to combine dynamic inputs and env vars, do you still need
? |
@ChrsMark Yes you still need to use |
/package |
Hey, is there anything missing with this so as to have it in and unblock #23679? |
2bfca38
to
df7a3c5
Compare
/package |
Tested locally with same built package and both Fleet mode and standalone work. So E2E test failed for an unknown reason. |
What does this PR do?
Fixes 2 issues that where affecting the usage of dynamic inputs and conditions.
inputs
from the go-ucfg variable expansion. Inside ofinputs
on variable expansion is done by the dynamic inputs code.*eql.null
.Why is it important?
Usage of dynamic inputs work as expected.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Related issues