Maintenance mode: check if domain is updated via API #229
Conversation
WalkthroughThe changes introduce a new step in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
User->>System: Request to start/stop maintenance workload
System->>System: Switch domain workload
System->>System: Check domain workload state
alt Domain workload matches expected state
System->>User: Confirmation of action
else Domain workload does not match
System->>System: Retry (max 3 attempts)
System->>User: Error if domain not found
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/core/maintenance_mode.rb (1 hunks)
Additional comments not posted (1)
lib/core/maintenance_mode.rb (1)
Line range hint
69-72: LGTM!The code changes are approved.
zzaakiirr
force-pushed
the
157-check-if-domain-was-updated-via-api-instead-of-sleeping
branch
from
8fe4f62 to
5ea8095
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/core/maintenance_mode.rb (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/core/maintenance_mode.rb
| @@ -84,9 +84,13 @@ def start_or_stop_maintenance_workload(action) | |||
| def switch_domain_workload(to:) | |||
| step("Switching workload for domain '#{domain_data['name']}' to '#{to}'") do | |||
| cp.set_domain_workload(domain_data, to) | |||
| end | |||
|
|
|||
| step("Waiting for changes to take effect", retry_on_failure: true, wait: 10, max_retry_count: 3) do | |||
There was a problem hiding this comment.
@borela should we be using https://github.com/shakacode/uber_task?
@zzaakiirr is this ready to merge?
There was a problem hiding this comment.
@justin808 I left a question for @rafaelgomesxyz, I want to get his opinion on approach I took in this PR for checking if domain was updated.
If this is the right way and Rafael approves this PR then we can merge this
|
|
||
| # Give it a bit of time for the domain to update | ||
| Kernel.sleep(30) | ||
| cp.domain_workload_matches?(refetched_domain_data, to) |
There was a problem hiding this comment.
@rafaelgomesxyz Is this check enough to be sure that domain was updated?
There was a problem hiding this comment.
The arbitrary sleep we had before wasn't really to check if the domain value was updated in the workload (after all, this should be true immediately when it first runs, no?).
The sleep was for waiting for DNS changes to take effect, which can sometimes take a bit. So we'd need a different method for checking it (we'd essentially need to check if the domain is working for the switched workload).
There was a problem hiding this comment.
So we'd need a different method for checking it
Do you have any ideas how can we check this? Maybe controlplane displays some message while workload change takes effect?
There was a problem hiding this comment.
@zzaakiirr A way I can think of is to make a real request to the domain and see if it's displaying the regular page or the maintenance page.
I'm not sure if there's a better way to do it.
|
|
||
| # Give it a bit of time for the domain to update | ||
| Kernel.sleep(30) | ||
| cp.domain_workload_matches?(refetched_domain_data, to) |
There was a problem hiding this comment.
The arbitrary sleep we had before wasn't really to check if the domain value was updated in the workload (after all, this should be true immediately when it first runs, no?).
The sleep was for waiting for DNS changes to take effect, which can sometimes take a bit. So we'd need a different method for checking it (we'd essentially need to check if the domain is working for the switched workload).
|
We'll need to find another way to check if domain is updated, see #229 (comment) |
What does this PR do?
This PR replaces
sleepwith actual API request to check ifworkloadLinkis changed for domain after updateTests
Specs for maintenance command are passing - https://github.com/shakacode/control-plane-flow/actions/runs/10626502289/job/29458117242
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes