feat(jans-linux-setup): activate proxy_http2 for jans-lock #13167
+30
−43
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Mustafa Baser <mbaser@mail.com>
devrimyatar
requested review from
iromli,
yurem,
yuriyz and
yuriyzz
as code owners
devrimyatar
added
kind-feature
Contributor
📝 WalkthroughWalkthroughRefactors Apache module management in HttpdInstaller: adds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
|
Error: Hi @devrimyatar, You did not reference an open issue in your PR. I attempted to create an issue for you. |
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@jans-linux-setup/jans_setup/setup_app/installers/httpd.py`:
- Line 154: Replace the non-idiomatic membership test "if not module in
mods_enabled:" with the PEP8-preferred form "if module not in mods_enabled:" in
the httpd installer code where the variables module and mods_enabled are used
(search for the membership check in jans_setup/setup_app/installers/httpd.py to
locate the conditional).
- Line 39: The enabled_modules list uses the wrong module name "mod_dir"; update
the entry in self.enabled_modules to "dir" so it matches Apache's module naming
(and the httpd_2.4.conf LoadModule dir_module pattern) and so the Debian check
for /etc/apache2/mods-available/{amod}.load will find the correct file; locate
the list assigned to self.enabled_modules in the httpd installer (the list
containing 'env', 'log_config', ..., 'rewrite', 'mod_dir', 'auth_openidc') and
replace 'mod_dir' with 'dir'.
Signed-off-by: Mustafa Baser <mbaser@mail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jans-linux-setup/jans_setup/setup_app/installers/httpd.py`:
- Around line 103-110: The enable_modules method uses copy.deepcopy on
self.enabled_modules unnecessarily; replace copy.deepcopy(self.enabled_modules)
with a shallow copy (e.g., list(self.enabled_modules) or
self.enabled_modules.copy()) to avoid overhead, keep the rest of the logic that
appends 'http2' and 'proxy_http2' when Config.install_jans_lock is true, and
remove the import copy if it is no longer referenced elsewhere.
Comment on lines
103
to
110
| def enable_modules(self): | ||
|
|
||
| mods_enabled = copy.deepcopy(self.enabled_modules) | ||
| if Config.install_jans_lock: | ||
| mods_enabled.append('http2') | ||
| mods_enabled.append('proxy_http2') | ||
|
|
||
| self.logIt(f"Apache modules to be enabled: {' '.join(mods_enabled)}") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use shallow copy instead of deepcopy for a flat list of strings.
copy.deepcopy() is unnecessary overhead here since self.enabled_modules is a flat list of immutable strings. A shallow copy is sufficient and more idiomatic.
♻️ Proposed fix
def enable_modules(self):
- mods_enabled = copy.deepcopy(self.enabled_modules)
+ mods_enabled = self.enabled_modules.copy()
if Config.install_jans_lock:
mods_enabled.append('http2')
mods_enabled.append('proxy_http2')With this change, the import copy at line 2 can also be removed if not used elsewhere.
🤖 Prompt for AI Agents
In `@jans-linux-setup/jans_setup/setup_app/installers/httpd.py` around lines 103 -
110, The enable_modules method uses copy.deepcopy on self.enabled_modules
unnecessarily; replace copy.deepcopy(self.enabled_modules) with a shallow copy
(e.g., list(self.enabled_modules) or self.enabled_modules.copy()) to avoid
overhead, keep the rest of the logic that appends 'http2' and 'proxy_http2' when
Config.install_jans_lock is true, and remove the import copy if it is no longer
referenced elsewhere.
Signed-off-by: Mustafa Baser <mbaser@mail.com>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #13128
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Closes #13168,
Summary by CodeRabbit