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

Add internet proxy config #470

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

fifofonix
Copy link
Contributor

@fifofonix fifofonix commented Sep 29, 2022

Concerns: #469

Also, adds a default nginx.conf file which is necessary for preview.sh to function out of the box for MacOS users.

@fifofonix fifofonix marked this pull request as ready for review September 29, 2022 21:23
Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM overall. Two small nits:

  • we use single space in the docs.
  • not sure you wanted to add that nginx.conf file here

nginx.conf Show resolved Hide resolved
@travier
Copy link
Member

travier commented Sep 30, 2022

Thanks for working on this!

@fifofonix
Copy link
Contributor Author

Thanks for working on this!

@travier you're welcome. Issues resolved. I'm not familiar with protocols here and whether I am expected to mark things above as resolved, or whether that is for you given you raised the questions?

@travier
Copy link
Member

travier commented Oct 1, 2022

Issues resolved. I'm not familiar with protocols here and whether I am expected to mark things above as resolved, or whether that is for you given you raised the questions?

I think you can mark them as resolved yourself, at least that's what I do.

@dustymabe
Copy link
Member

Thanks for working on this @fifofonix - looking good to me. Let's get that nginx.conf file broken out into a separate commit and then the extraneous two commits that you have right now squashed. At the end I'm thinking two commits:

  • Add nginx.conf file
  • Add internet proxy config doc

@fifofonix fifofonix force-pushed the add-internet-proxy-config branch from fb1b784 to 1c69425 Compare October 4, 2022 22:56
@dustymabe
Copy link
Member

This LGTM - only thing I would add is maybe linking to the original location where the nginx.conf file was copied from in the commit message for that commit.

@travier do you want to do one more review pass?

@fifofonix fifofonix force-pushed the add-internet-proxy-config branch from 1c69425 to c689f67 Compare October 5, 2022 20:36
@fifofonix
Copy link
Contributor Author

This LGTM - only thing I would add is maybe linking to the original location where the nginx.conf file was copied from in the commit message for that commit.

@travier do you want to do one more review pass?

Done

@travier
Copy link
Member

travier commented Oct 5, 2022

If you can update your commit message to mention https://gitlab.com/fedora/docs/templates/fedora-docs-template/-/blob/main/nginx.conf instead as it's the template repo that would be great. LGTM otherwise.

Required for ./build.sh && ./preview.sh on MacOS (file copied from https://gitlab.com/fedora/docs/templates/fedora-docs-template/-/blob/main/nginx.conf)
@fifofonix fifofonix force-pushed the add-internet-proxy-config branch from c689f67 to f585e49 Compare October 6, 2022 10:20
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe dustymabe merged commit 2f829b1 into coreos:main Oct 6, 2022
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.

3 participants