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

[Elastic Agent] Inconsistency for enroll and install command #21897

Closed
ruflin opened this issue Oct 16, 2020 · 8 comments · Fixed by #23865
Closed

[Elastic Agent] Inconsistency for enroll and install command #21897

ruflin opened this issue Oct 16, 2020 · 8 comments · Fixed by #23865

Comments

@ruflin
Copy link
Collaborator

ruflin commented Oct 16, 2020

The usage of our enrollment command looks as following:

elastic-agent enroll <kibana_url> <enrollment_token> [flags]

For install it is:

elastic-agent install [flags]

To pass enrollment_token and kibana_url the flags --kibana-url and --enrollment-token are needed but these two flags are missing for enroll. We should be consistent here.

I stumbled over this when I copy pasted the install command but only wanted to enroll so I change install to enroll and it failed as enroll did not have the flags. I like the flags in general as they don't enforce a certain oder so I think we should make the flags also available for enroll to be consistent.

In addition, it should be checked if there are further incosistencies.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@blakerouse
Copy link
Contributor

blakerouse commented Oct 16, 2020

Well I think the inconsistency has validity. That is because for install kibana-url and enrollment-token are optional where as for the enroll command they are required.

@ruflin
Copy link
Collaborator Author

ruflin commented Oct 16, 2020

Would it make sense to at least support in enroll to also pass it as flag in addition? Not even sure if this is possible to have both option 🤔

@blakerouse
Copy link
Contributor

It is possible to make the enroll command use those parameters and those parameters be required. Removing the requirement of positional arguments on the enroll command all together.

@ph
Copy link
Contributor

ph commented Oct 16, 2020

My question would, be does it worth the effort and what we gain? I believe we want to encourage the install workflow? IMHO enroll only makes sense now for testing and even with that, not everything can be tested. Which open actually opens a few more questions.

  • Should we keep enroll?
  • Should we keep enroll only for testing and hide from --help or special build.

@blakerouse
Copy link
Contributor

You need to keep enroll in the case of DEB/RPM installation, where the install command cannot be used.

@ph
Copy link
Contributor

ph commented Oct 16, 2020

🤦 oh deb/rpm :(

@ruflin
Copy link
Collaborator Author

ruflin commented Oct 19, 2020

If I understand the comment from @blakerouse correctly, we can only move to the params for enroll if we remove the positional arguments. As this is a breaking change, I would delay this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants