-
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
[Metricbeat] Remove make kibana command #12440
Conversation
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.
LGTM
Can you double check that also after the removal the directory _meta/kibana.generated is still created?
@@ -50,7 +43,7 @@ imports: python-env | |||
|
|||
# Runs all collection steps and updates afterwards | |||
.PHONY: collect | |||
collect: assets collect-docs configs kibana imports | |||
collect: assets collect-docs configs imports |
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.
Is this was still called by collect, I wonder why it still worked? I assume there is an other command (inside mage update)? which overwrites this afterwards?
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.
Good catch. My mistake, I was looking at build
folder (which is where they are generated for xpack, instead of _meta/kibana
.
The problem arised because few os us have had few internal chats over the last months about not being able to load dashboards because we were using make kibana
command directly. Right now, it's not expected that you run make kibana
by yourself.
make kibana
by itself does nothing right now and leads to confusion. It's only required by update
to decode/encode them in this script https://github.com/elastic/beats/blob/master/libbeat/scripts/unpack_dashboards.py which mutates them in place.
So, I'd still like to remove the command because it's not intended to be used directly by the developer. I see 2 options:
- Modify
unpack_dashboards.py
so it doesn't mutate files but, instead, writes them into a single destination folder (as required bymetricbeat setup
) - Move the commands of
make kibana
in this PR into theimport-dashboards
command.kibana.generated
files aren't necessary if you aren't importing dashboards so I don't see why we need them intoupdate
command. I found that they are used here https://github.com/elastic/beats/blob/master/libbeat/scripts/Makefile#L360
WDYT?
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.
What about option 3: Move this logic to mage update
as we have in Filebeat and have then the files under build/kibana
? This would mean we can remove it completely here (I hope). Unfortunately don't know how many traps are hidden to get this done.
I'm just worried we try to fix something that should disappear anyways.
Be aware that some of the commands are also required to have the correct content for packaging. So it's not necessarly only import-dashboards
that needs the files there.
Also I pretty often run Metricbeat locally with -E setup.dashboards.directory=_meta/kibana.generate
to play around with the setup command.
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.
Sounds good but mage update
calls make update
in Metricbeat 😄 I have checked and we still need quite a lot of work to have mage update
. I just took an overall look at it however.
It would be completely out of the scope of this PR. Maybe we can, at least, echo a message about not using this directly or update the comment on Makefile temporarly? I'm happy to work on mage update
command but we need to prioritize it first with the rest of our roadmap 😄
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.
Adding a message SGTM. Sorry I'm pushing back on this change but I'm a bit worried about the effect on packaging and the things we haven't thought of.
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.
FYI: There is a mage target for loading dashboards. It's just not applied to all projects. I only added it to Winlogbeat. Adding
// mage:import
_ "github.com/elastic/beats/dev-tools/mage/target/dashboards"
establishes mage dashboards:import
and mage dashboards:export
targets.
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 have been looking at importing the dashboards
command from mage. It's not trivial but, in any case, it still requires make update
command to work and so make kibana
too 😅
I think it's better to open a new PR to add mage commands slowly.
About the message, just an echo saying Running "make kibana". This make target is deprecated and will be removed soon. This message should only appear when running "make update". Use "metricbeat setup --dasboards" if you want to import dashboards into Kibana"
I took a look at the docs and there's no mention to this command anymore so I guess that this sad PR is just closing 😄 . Thanks for the comments! |
Recently, I realized that
make kibana
command is still available in the Metricbeat Makefile but that it's invalid after we merged #7224Remove the command from the Makefile and all its reference as requirement.
I have checked that you can still import dashboards correctly with
make import-dashboards
but definitely it doesn't work withmake kibana