Skip to content

document change#8641

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
dichn:master
Oct 23, 2019
Merged

document change#8641
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
dichn:master

Conversation

@dichn
Copy link
Copy Markdown
Contributor

@dichn dichn commented Oct 17, 2019 •
edited
Loading

Description: Fix Document issue #8549
Risk Level: Low
Testing:
Docs Changes: Yes
Release Notes:
[Optional Fixes #Issue] Document issue #8549
[Optional Deprecated:]

@dichn
document change
3ba6713 
Signed-off-by: Di Chen <dichen@redhat.com>
jmarantz
jmarantz reviewed Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good; can you just update the PR description? I assume this is because the previous instructions don't really work and this makes them work?

Are you sure that's across all versions of Fedora?

@lizan for visibility.

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 17, 2019

Hi @jmarantz

After the change, I compiled Envoy successfully on Fedora 30. Otherwise will have issue #8549.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 17, 2019

Sounds good. Is this true for other Fedora releases? Or is 30 the most recent one?

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 17, 2019

I am not sure about other release, and 30 is the latest one.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 17, 2019

Maybe update the doc to reflect that these instructions work on Fedora 30?

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 18, 2019 •
edited
Loading

Hi @jmarantz
I find the issue also exists in Fedora 29

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 18, 2019

I think your change is basically fine but i have to wonder why whoever wrote the fedora instructions got it wrong. That's why I thought maybe it's version-specific?

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 18, 2019

Yes, understood, but I think the other release will have the same problem :), I will try to find out

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 18, 2019 •
edited
Loading

Hi @jmarantz , other releases have same problem, it's not a version-specific problem, I think we can just add this library to the dependencies list

dichn added 2 commits October 18, 2019 20:30
@dichn
fix issue #8642
886a0a0 
Signed-off-by: Di Chen <dichen@redhat.com>
@dichn
change about #8641 and #8642
38d5aaa 
Signed-off-by: Di Chen <dichen@redhat.com>
jmarantz
jmarantz approved these changes Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks different from before. Do they both work? Or is it hard to tell now you are not starting from a clean system?

In any case I'm OK with this but you might want to be more transparent in the comments about what you discovered for the benefits of other users.

But I'll defer to @lizan and also over-to-him for senior maintainer review.

@dichn
Copy link
Copy Markdown
Contributor Author

dichn commented Oct 18, 2019 •
edited
Loading

Hi,
I just start to contribute Envoy, my environment is pretty clean. I am sure about that. I found this issue when I tried to deploy and test in my first time.

The changes both work!

The final patch related to another patch #8642

@mattklein123 mattklein123 merged commit 8438e37 into envoyproxy:master Oct 23, 2019
spenceral added a commit to spenceral/envoy that referenced this pull request Oct 23, 2019
@spenceral
Merge branch 'master' into downstream-protocol-error
e4eb45c 
* master: (54 commits)
  Update dependencies - Go, Bazel toos, xxHash, nanopb, rules_go, curl, protobuf, Jinja, yaml-cpp, re2 (envoyproxy#8728)
  test: increase coverage of listener_manager_impl.cc (envoyproxy#8737)
  test: modify some macros to reduce number of uncovered lines reported (envoyproxy#8725)
  build: add a script to setup clang (envoyproxy#8682)
  http: fix ssl_redirect on external (envoyproxy#8664)
  docs: update fedora build requirements (envoyproxy#8641)
  fix draining listener removal logs (envoyproxy#8733)
  dubbo: fix router doc (envoyproxy#8734)
  server: provide server health status when stats disabled (envoyproxy#8482)
  router: adding a knob to configure a cap on buffering for shadowing/retries (envoyproxy#8574)
  tcp proxy: add default 1 hour idle timeout (envoyproxy#8705)
  thrift: fix filter names in docs (envoyproxy#8726)
  Quiche changes to avoid gcc flags on Windows (envoyproxy#8514)
  test: increase test coverage in Router::HeaderParser (envoyproxy#8721)
  admin: add drain listeners endpoint  (envoyproxy#8415)
  buffer filter: add content-length when ending stream with trailers (envoyproxy#8609)
  clarify draining option docs (envoyproxy#8712)
  build: ignore go-control-plane mirror git commit error code (envoyproxy#8703)
  api: remove API type database from checked in artifacts. (envoyproxy#8716)
  admin: correct help strings (envoyproxy#8710)
  ...

Signed-off-by: Spencer Lewis <slewis@squareup.com>
derekargueta pushed a commit to derekargueta/envoy that referenced this pull request Oct 24, 2019
@dichn @derekargueta
docs: update fedora build requirements (envoyproxy#8641)
627df87 
Signed-off-by: Di Chen <dichen@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmarantz jmarantz jmarantz approved these changes

Assignees

@lizan lizan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dichn @jmarantz @lizan @mattklein123