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

consul: fix to remove outdated caveat #15926

Closed
wants to merge 1 commit into from

Conversation

leepa
Copy link
Contributor

@leepa leepa commented Jul 23, 2017

The caveat for the -ui-dir parameter is no longer correct as since 0.7
and above produce this warning if you try and use it:

==> If using Consul version 0.7.0 or later, the web UI is included in the
    binary so use ui to enable it

Using -agent -dev implies -ui and so the caveat produces an error trying
to start Consul.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@ilovezfs
Copy link
Contributor

Should we also remove

 pkgshare.install "ui" => "web-ui"

in that case?

@leepa
Copy link
Contributor Author

leepa commented Jul 23, 2017

@ilovezfs I'll do some tests to make sure that's correct and update this shortly if that's valid/or not.

The caveat for the `-ui-dir` parameter is no longer correct as since 0.7
and above produce this warning if you try and use it:

```
==> If using Consul version 0.7.0 or later, the web UI is included in the
    binary so use ui to enable it
```

Using -agent -dev implies -ui and so the caveat produces an error trying
to start Consul.

As a result the `pkgshare.install` is superfluous and so that's gone
too.
@leepa
Copy link
Contributor Author

leepa commented Jul 23, 2017

Amended to remove the pkgshare.install as yes, with the UI compiled in it is no longer required.

@ilovezfs
Copy link
Contributor

@leepa is this going to be a problem, though: hashicorp/consul#3292 (comment)

@leepa
Copy link
Contributor Author

leepa commented Jul 23, 2017

Depends if one wants to locally customise - which I'd recommend against doing in that folder as it'll be overwritten by a subsequent install, surely? Is it better for the user to download and customise the theme outside of /usr/local?

@ilovezfs
Copy link
Contributor

@leepa I guess we should just remove it, since that particular case will be fixed in the next release anyway.

@leepa
Copy link
Contributor Author

leepa commented Jul 23, 2017

I believe the priority, in this case, is to ensure that users aren't confused by the caveat when they inevitably upgrade.

Whatever happens in the Consul project it'll need new guidance once they've fixed the issue, so I believe the current state is correct. Let me know if there's anything else I need to do.

@ilovezfs
Copy link
Contributor

Nope, shipped! Thanks for the PR.

@ilovezfs ilovezfs closed this in 45cae60 Jul 23, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants