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

Restore working state by refactoring Makefile and Nginx config #537

Open
wants to merge 2 commits into
base: ubuntu-upgrade
Choose a base branch
from

Conversation

bullet-ant
Copy link

  • Fix improper indentations in Makefile
  • Fix Dockerfile to adhere to standards
  • Remove ssl on from Nginx configurations

Copy link

google-cla bot commented Sep 3, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bullet-ant bullet-ant changed the title Refactor Makefile and Nginx Config to Restore Working Setup Restore working state by refactoring Makefile and Nginx config Sep 3, 2024
@lgarron
Copy link
Collaborator

lgarron commented Sep 3, 2024

Do you have more context on why these changes are necessary/worthwhile?

Also, we have to use an old version of nginx for old SSL, do you foresee any issues with that?

@bullet-ant
Copy link
Author

bullet-ant commented Sep 4, 2024

Thank you for reviewing the PR, @lgarron

Regarding the necessity of these changes:

  1. Makefile Indentation: I had to correct indentation issues in the Makefile, which were causing build errors.
  2. Dockerfile Adjustments: I removed the RUN gem update --system command because it caused an error during the Docker build process. The error was due to RubyGems being installed through APT, which does not support upgrading via RubyGems itself.
  3. Nginx Configuration: The removal of the ssl on directive is necessary because it’s deprecated in the Nginx version included with Ubuntu 24.04. The newer Nginx version uses listen ... ssl instead. Since we’re not specifying a version in the Dockerfile, apt installs the latest version available in the Ubuntu repositories, which requires this change.

Regarding your concern about using an older version of Nginx for old SSL:

If we need to maintain compatibility with legacy SSL protocols, we could consider pinning Nginx to an older version that supports ssl on. However, doing so may introduce security risks, as older versions might not have the latest security patches. I’d be happy to explore this further if you think it’s necessary.

Thanks again for your guidance, and I look forward to your feedback!

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.

2 participants