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

Host header update #565

Merged
merged 15 commits into from
May 3, 2020
Merged

Conversation

johnk-novu
Copy link
Contributor

@johnk-novu johnk-novu commented Mar 31, 2020

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

Go's http ignores/removes certain headers (n1, n2), including but not limited to the Host header. See: github issue and go source

This change detects the presence of that header (or headers) and sets it on the http request.

Issue opened: #564

I also took the liberty of making a couple of unrelated changes:

  • update README.md
  • rename a duplicate test in goss-shared.yml

@johnk-novu johnk-novu closed this Mar 31, 2020
@johnk-novu johnk-novu reopened this Mar 31, 2020
integration-tests/goss/goss-shared.yaml Outdated Show resolved Hide resolved
system/http.go Outdated Show resolved Hide resolved
system/http.go Outdated Show resolved Hide resolved
@johnk-novu
Copy link
Contributor Author

Confirmed tests pass, I think this is good to go.

Copy link
Contributor

@pedroMMM pedroMMM left a comment

Choose a reason for hiding this comment

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

@aelsabbahy it's ready for you.

Copy link
Member

@aelsabbahy aelsabbahy left a comment

Choose a reason for hiding this comment

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

Small change requested.

Also, can you please update the docs to clarify that only the first Host: foo value is honored.

Aside from that, looks great!

system/http.go Outdated Show resolved Hide resolved
integration-tests/goss/goss-shared.yaml Show resolved Hide resolved
integration-tests/goss/goss-shared.yaml Show resolved Hide resolved
@johnk-novu
Copy link
Contributor Author

@aelsabbahy I made the documentation update, let me know what you think!

cd4aeb3

@aelsabbahy
Copy link
Member

Looks great. There's going to be a bit of juggling with other PRs.. but I should have this merged/released in the next ~2weeks.

@johnk-novu
Copy link
Contributor Author

@aelsabbahy awesome thanks! looking forward to it, this will allow me to implement a very helpful test :)

@aelsabbahy aelsabbahy merged commit 7176a7d into goss-org:master May 3, 2020
@aelsabbahy
Copy link
Member

Awesome change, thank you!

BenjaminHerbert pushed a commit to BenjaminHerbert/goss that referenced this pull request May 28, 2020
* readme update

* set host properly header if present

* uncomment fix; add test

* add back os lib

* remove printing to stderr/stdout

* rename host header test

* rename other duplicate test

* remove unneeded lib

* update test counts

* ninja markdown edit

* Update system/http.go

Co-Authored-By: Ahmed Elsabbahy <aelsabbahy@users.noreply.github.com>

* add note about host header

* Revert documentation change since it was fixed with goss-org#582

Co-authored-by: Ahmed Elsabbahy <aelsabbahy@users.noreply.github.com>
Co-authored-by: Ahmed Elsabbahy <elsabbahyahmed@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants