vishesh92 commented on PR #105: URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/105#issuecomment-5634540786
> @vishesh92 can you check the below. > > * Duplicate-rule cleanup assumes same public IP (please confirm or guard) > In _getLoadBalancer_, a name-collided rule hits continue before the _lb.ipAddr_ consistency check, and _deleteDuplicateRules_ only deletes the LB rule. If a duplicate ever had a different public IP, its firewall rule / IP would leak on cleanup — the exact class of bug this PR fixes elsewhere. Can we either add a guard, or a one-line comment confirming duplicates are guaranteed to share IP/port/protocol? > > * Guard the e2e job like the build job > build is gated on _if: github.repository == 'apache/...' || workflow_dispatc_h, but e2e (needs: build) has no matching if. A workflow_dispatch from a fork (or a repo rename) would leave e2e running with no artifact to download. Also cache-to: type=gha,mode=max is read-only on fork PRs and will warn. Suggest mirroring the if: onto e2e. > > * make test now runs against the live simulator in CI > Sourcing _keys.env_ before make test activates the acceptance tests in _cloudstack_test.go_ against the simulator. Intentional and useful, but it couples the unit-test signal to simulator health. Worth confirming that's the intent — or splitting the pure-unit run from the live run so a sim hiccup doesn't read as a code failure. > > * Minor nits > > * _.gitignore_ is missing a trailing newline. > * _retry()_ in hack/e2e/lib/log.sh looks unused — drop it? > * _docs/development.md_ says "Go 1.23 or later" while CI uses go-version-file: go.mod — keep in sync with go.mod. > * _majorMinorPatch_ is duplicated in cloudstack.go and test/e2e/framework.go (unavoidable across the package boundary) — worth a note so a future edit updates both. I have addressed these. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
