Skip to content

fix(tomcat): align template standards#686

Open
mberlofa wants to merge 2 commits into
mainfrom
fix/tomcat-template-standards
Open

fix(tomcat): align template standards#686
mberlofa wants to merge 2 commits into
mainfrom
fix/tomcat-template-standards

Conversation

@mberlofa

@mberlofa mberlofa commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep the Helm test hook under templates/tests so helm test remains functional
  • add networkPolicy.egress.extraEgress support with schema and unit coverage
  • expand NOTES into numbered operational sections
  • update README defaults for the pinned Tomcat image tag and new egress control

Validation

  • make validate-chart CHART=tomcat TIMEOUT=900: FULLY VALIDATED (16 layers), including k3d behavioral scenarios for default, gateway-api, hardening, ingress, ip-family-policy, JMX, and standalone.
  • make site-sync-check CHART=tomcat
  • make release-check REPO=charts passed with the expected GR-077 post-merge release confirmation warning.
  • make attribution-check REPO=charts

Site PR: helmforgedev/site#364
Issue: #633

Summary by CodeRabbit

  • Documentation
    • Updated the Tomcat chart README to use the newer default “Official Tomcat image” tag.
    • Reformatted and expanded chart NOTES with clearer, numbered sections and more detailed output for JMX and operational steps.
  • New Features
    • Added networkPolicy.egress.extraEgress to support supplying additional complete NetworkPolicy egress rules.
  • Bug Fixes
    • Improved NetworkPolicy egress rendering to correctly combine built-in settings with extraEgress, including when base egress is disabled.
  • Tests
    • Added an operations test for egress when base egress is disabled and extraEgress is provided.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Standards Check (GR-079) — PASS

Every changed chart fully passes standards-check.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2af0a3b4-1878-4ec4-a62b-8ae4516ceb61

📥 Commits

Reviewing files that changed from the base of the PR and between d1854d2 and a0ef176.

📒 Files selected for processing (1)
  • charts/tomcat/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • charts/tomcat/README.md

📝 Walkthrough

Walkthrough

Adds networkPolicy.egress.extraEgress support in the Tomcat chart’s schema, defaults, template, test, and README, and restructures NOTES.txt into numbered sections with expanded release, service, health, validation, JMX, and production output.

Changes

NetworkPolicy extraEgress feature

Layer / File(s) Summary
Values schema and defaults for extraEgress
charts/tomcat/values.schema.json, charts/tomcat/values.yaml
Expands the networkPolicy.egress schema with allow flags and extraTo/extraEgress array fields, and adds a default empty extraEgress value.
NetworkPolicy template egress logic
charts/tomcat/templates/networkpolicy.yaml
Introduces $egress, $extraEgress, and $hasBaseEgress locals; updates the egress rendering condition to trigger on base egress or extraEgress presence; reworks rule emission to append extraEgress with an empty-list fallback.
Operations test and README updates
charts/tomcat/tests/operations_test.yaml, charts/tomcat/README.md
Adds an operations test validating extraEgress rendering when base egress is disabled, and documents extraEgress usage plus the bumped 11.0.23-jdk17-temurin-noble image tag in the README.

NOTES.txt restructuring

Layer / File(s) Summary
NOTES.txt numbered sections
charts/tomcat/templates/NOTES.txt
Restructures the post-install NOTES output into numbered Release, Service, Workload, Health, Validation, JMX, Production reminders, and Documentation sections, adding Gateway API hostnames and expanded JMX/production guidance.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

  • helmforgedev/charts#641: Also extends networkpolicy.yaml to support networkPolicy.egress.extraEgress, with matching schema, values, docs, and rendering tests.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is clearly related to the chart-standardization changes across templates, notes, README, and network policy.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/tomcat-template-standards

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🟢 Security Scan: tomcat

Framework Score
MITRE + NSA + SOC2 94.94949%

✅ Security posture acceptable.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2dbaa7dc58

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread charts/tomcat/templates/NOTES.txt

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
charts/tomcat/README.md (1)

82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider clarifying extraTo vs extraEgress distinction.

Both extraTo (appends peers to base egress rules) and extraEgress (adds standalone complete rules) are documented separately, but the Production Notes bullet doesn't clarify when to use one over the other, which could confuse users who need only additional peers rather than an independent rule.

Also applies to: 107-107

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/tomcat/README.md` at line 82, Clarify the Production Notes guidance in
the README by explaining the difference between networkPolicy.egress.extraTo and
networkPolicy.egress.extraEgress: use extraTo in the README’s egress note when
users want to append additional peers to the existing base egress rules, and
reserve extraEgress for defining standalone custom egress rules. Update the
bullet near the networkPolicy.egress section so users can choose the right
option without ambiguity, and make the same clarification in the other
referenced note as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@charts/tomcat/README.md`:
- Line 82: Clarify the Production Notes guidance in the README by explaining the
difference between networkPolicy.egress.extraTo and
networkPolicy.egress.extraEgress: use extraTo in the README’s egress note when
users want to append additional peers to the existing base egress rules, and
reserve extraEgress for defining standalone custom egress rules. Update the
bullet near the networkPolicy.egress section so users can choose the right
option without ambiguity, and make the same clarification in the other
referenced note as well.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 16a9c508-b18f-4b7f-902e-1dade2c0cdc7

📥 Commits

Reviewing files that changed from the base of the PR and between 47bc2fc and 2dbaa7d.

📒 Files selected for processing (8)
  • charts/tomcat/README.md
  • charts/tomcat/templates/NOTES.txt
  • charts/tomcat/templates/networkpolicy.yaml
  • charts/tomcat/tests/operations_test.yaml
  • charts/tomcat/tests/test-connection.yaml
  • charts/tomcat/tests/testconnection_test.yaml
  • charts/tomcat/values.schema.json
  • charts/tomcat/values.yaml
💤 Files with no reviewable changes (1)
  • charts/tomcat/tests/testconnection_test.yaml

@mberlofa mberlofa force-pushed the fix/tomcat-template-standards branch from 396abf6 to d1854d2 Compare July 4, 2026 20:33
@mberlofa

mberlofa commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the still-valid CodeRabbit review-body nitpick:

  • charts/tomcat/README.md: clarified that networkPolicy.egress.extraTo appends peers to the built-in egress rules, while networkPolicy.egress.extraEgress is for standalone complete egress rules.
  • Synced the same distinction in the paired site PR: docs(tomcat): sync template standards updates site#364.

Validation completed after the fix:

  • make validate-chart CHART=tomcat TIMEOUT=1200 passed: tomcat: FULLY VALIDATED (16 layers) including all GR-027 k3d scenarios.
  • make release-check REPO=charts passed with only the expected GR-077 release warning.
  • make attribution-check REPO=charts passed.

This CodeRabbit note was in the review summary body, not as a separate resolvable review thread, so there was no thread ID to resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant