Conversation
|
@mattgallagher92 this is ready for review |
mattgallagher92
left a comment
There was a problem hiding this comment.
Is this field optional? If so, this change will break any existing clients, so we should probably make the new field optional too. If it's mandatory then there can't be any existing clients so we don't have to worry about that, in which case this change is fine (but I've added some comments about some things to improve).
There was a problem hiding this comment.
Should one of the tests check that the value is set correctly?
There was a problem hiding this comment.
Added assertions to check that queueName, messageCount, and namespace are all correctly serialized in the JSON output (commit 47bfd399).
There was a problem hiding this comment.
Do the docs need updating too?
There was a problem hiding this comment.
Updated container-apps.md to document all four fields of ServiceBusScaleRule (QueueName, Namespace, MessageCount, SecretRef) (commit 47bfd399).
|
@Larocceau sorry for the delay, are there any updates on the docs and testing so this can get completed? Please @ me if you need more information. |
|
Hi @ninjarobot! Sorry for the delay; things been a bit messy for us, and I've left CIT (as has most other dev staff). I did not get around this, but would be happy to wrap this up later. |
|
@copilot resolve the merge conflicts in this pull request |
…to namespace-for-sbus-scale-rules # Conflicts: # RELEASE_NOTES.md Co-authored-by: ninjarobot <1520226+ninjarobot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/CompositionalIT/farmer/sessions/7d45065b-a4c6-4a71-8b8d-d805f0d13ad7 Co-authored-by: ninjarobot <1520226+ninjarobot@users.noreply.github.com>
Merged master into the PR branch and resolved the conflict in |
|
@copilot also need to run fantomas on the .fs files that are updated in this PR. |
This PR closes #1188
The changes in this PR are as follows:
I have read the contributing guidelines and have completed the following:
If I haven't completed any of the tasks above, I include the reasons why here:
Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure: