Conversation
97eafa3 to
90bd33f
Compare
|
FYI release flow has been tested with pre-release v1.3.2-beta.1. |
|
|
||
| RUN make build | ||
|
|
||
| FROM alpine:latest |
There was a problem hiding this comment.
uh oh, of course not. Nice catch 🤦
Fixed it by pinning it to the latest patch version of release 3.23 (same as csi-cloudscale).
| IdleConnTimeout: 90 * time.Second, | ||
| MaxIdleConns: 50, | ||
| MaxIdleConnsPerHost: 50, | ||
| MaxConnsPerHost: 0, |
There was a problem hiding this comment.
I see we are also left this at 0 in the other projects, but could we potentially prevent big spikes if we added a sensible cap, like with MaxConnsPerHost: 50?
There was a problem hiding this comment.
Personally, I think we should not limit the amount of connections here: the deployment of that controller is not in our care and IMO we should use sensible but not overly limiting defaults. The default is 0, so previous to this change we didn't set it anyway. I merely added this to make it transparent what is set.
Rate-limiting is a feature of the backend.
| |------------------------|------------------------------| | ||
| | Any `*.go` | `make lint-fix && make test` | | ||
| | `go.mod` / `go.sum` | `make test` | | ||
| | `.github/workflows/` | Verify with zizmor | |
There was a problem hiding this comment.
zizmor is only available in the CI. Could we add a make target?
There was a problem hiding this comment.
I removed that entry - installing zizmor in a cross-os compatible way is not something I want to do (so it works also e.g. in agent sandboxes). We do have zizmor as a github action and if wished, one can brew install it for local fixing.
| build-args: | | ||
| VERSION=${{ env.TAG }} | ||
| GIT_COMMIT=${{ github.sha }} | ||
| BUILD_DATE=${{ github.event.head_commit.timestamp }} |
There was a problem hiding this comment.
Are we sure we can relay on github.event.head_commit.timestamp here? It may be unavailable for some workflow triggers.
There was a problem hiding this comment.
this workflow is only triggered upon creating a tag and there we have it for sure.
See also the "Build inputs" here: https://github.com/cloudscale-ch/cloudscale-cloud-controller-manager/actions/runs/35830854988
| token string, timeout time.Duration) *cloudscale.Client { | ||
| // NewTransport creates an http.Transport configured for the cloudscale.ch API. | ||
| // The returned transport should be created once and shared across all clients | ||
| // to benefit from connection pooling and HTTP/2 multiplexing. |
There was a problem hiding this comment.
Maybe I'm missing something, but newCloudscaleClient() calls NewTransport() for every client instantiation.
There was a problem hiding this comment.
True that, I copied that over verbatim from capcs where we do a separate transport setup and have for each reconciliation a new client. In CCM we don't have this - we just have one client for the entire duration of the CCM.
I removed the sentence to avoid confusion.
90bd33f to
56ab719
Compare
2 participants
What changed: