Fix issue where cleanup-stale-apps command fails to delete apps with volumesets
#175
Conversation
WalkthroughThe recent update fixes a bug in the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| return progress.puts("Workload '#{workload}' does not exist.") if cp.fetch_workload(workload).nil? | ||
| if cp.fetch_workload(workload).nil? | ||
| return progress.puts("Workload '#{workload}' does not exist in app '#{config.app}'.") | ||
| end |
There was a problem hiding this comment.
All changes in this file are simply to add, e.g., " app 'my-app'", at the end of the progress messages, so that it's clear which app each item being deleted belongs to.
| end | ||
| end | ||
| def delete_app(app) | ||
| Cpl::Cli.start(["delete", "-a", app, "--yes"]) |
There was a problem hiding this comment.
Simply call the delete command instead of implementing its own delete methods.
It's ok to pass --yes here because this is only called after confirm_delete:
control-plane-flow/lib/command/cleanup_stale_apps.rb
Lines 28 to 32 in 50930f7
So the confirmation step will already have been handled by the time we get here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (2)
lib/command/cleanup_stale_apps.rb (1)
Line range hint
44-61: Refactor long method for readability and maintainability.Consider breaking down the
stale_appsmethod into smaller, more manageable methods to improve readability and maintainability.def stale_apps @stale_apps ||= begin apps = [] now = DateTime.now stale_app_image_deployed_days = config[:stale_app_image_deployed_days] gvcs = cp.gvc_query(config.app)["items"] gvcs.each do |gvc| app_name = gvc["name"] image = latest_image_for_app(app_name) next unless image created_date = DateTime.parse(image["created"]) diff_in_days = (now - created_date).to_i next unless diff_in_days >= stale_app_image_deployed_days apps.push({ name: app_name, date: created_date }) end apps end end private def latest_image_for_app(app_name) images = cp.query_images(app_name)["items"].select { |item| item["name"].start_with?("#{app_name}:") } latest_image_from(images, app_name: app_name, name_only: false) endCHANGELOG.md (1)
Line range hint
116-116: Correct the operating system name.The operating system from Apple should be written as "macOS".
- Fixed failed build on MacOS by adding platform flag and fixed multiple files in yaml document for template. + Fixed failed build on macOS by adding platform flag and fixed multiple files in yaml document for template.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- docs/commands.md (1 hunks)
- lib/command/cleanup_stale_apps.rb (4 hunks)
- lib/command/delete.rb (5 hunks)
- spec/command/cleanup_stale_apps_spec.rb (3 hunks)
- spec/command/delete_spec.rb (8 hunks)
Files not reviewed due to errors (4)
- lib/command/delete.rb (no review received)
- spec/command/cleanup_stale_apps_spec.rb (no review received)
- spec/command/delete_spec.rb (no review received)
- docs/commands.md (no review received)
Additional Context Used
LanguageTool (22)
CHANGELOG.md (11)
Near line 21: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [2.0.0] - 2024-05-14 ### BREAKING CHANGES - C...
Near line 51: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.4.0] - 2024-03-20 ### Added - Added new te...
Near line 66: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.3.0] - 2024-03-19 ### Fixed - Fixed issue ...
Near line 88: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.2.0] - 2024-01-03 ### Fixed - Fixed issue ...
Near line 116: The operating system from Apple is written “macOS”.
Context: ...-17 ### Fixed - Fixed failed build on MacOS by adding platform flag and fixed multi...
Near line 134: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.1.0] - 2023-09-20 ### Fixed - Fixed issue ...
Near line 152: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.0.4] - 2023-07-21 ### Fixed - Fixed issue ...
Near line 158: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.0.3] - 2023-07-07 ### Fixed - Fixedrun...
Near line 172: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.0.2] - 2023-07-02 ### Added - Added steps ...
Near line 179: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.0.1] - 2023-06-28 ### Fixed - Fixed `clean...
Near line 189: Unpaired symbol: ‘[’ seems to be missing
Context: ...//github.com/rafaelgomesxyz). ## [1.0.0] - 2023-05-29 - Initial release [Unrel...docs/commands.md (11)
Near line 56: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...uantity or are older than the specified amount of days - Specify the max quantity thro...
Near line 57: Possible verb agreement error. Did you mean “specifies”? (Some collective nouns can be treated as both singular and plural, so ‘Specify’ is not always incorrect.)
Context: ...der than the specified amount of days - Specify the max quantity through `image_retenti...
Near line 58: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ne/controlplane.ymlfile - Specify the amount of days throughimage_retention_days` ...
Near line 72: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... date of the latest image - Specify the amount of days after an app should be consider...
Near line 126: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...fcpl runwith the latest image - The deploy will fail if the release script exits w...
Near line 212: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Near line 223: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Near line 234: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
Near line 279: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...he--run-release-phaseoption - The deploy will fail if the release script exits w...
Near line 358: Unpaired symbol: ‘"’ seems to be missing
Context: ...t isbash, in which case the args ["-c", cmd_to_run] are passed - Providing `--...
Near line 410: Consider replacing ‘only’ with a different word to let your writing stand out.
Context: ...ne/controlplane.yml` file - This should only be used for temporary apps like review ...
Additional comments not posted (1)
lib/command/cleanup_stale_apps.rb (1)
Line range hint
68-72: LGTM! Theconfirm_deletemethod is straightforward and does not require changes.
The
cleanup-stale-appscommand is currently outdated compared to thedeletecommand, in terms of deleting apps with volumesets.This PR fixes that by changing the
cleanup-stale-appscommand to simply call thedeletecommand for each app, instead of implementing its own delete methods. That ensures that whatever changes are made to thedeletecommand are reflected in thecleanup-stale-appscommand.Summary by CodeRabbit
Bug Fixes
cleanup-stale-appscommand failed to delete apps with volumesets.New Features
cleanup-stale-appscommand to unbind apps from the secrets policy if both identity and policy exist.Documentation
cleanup-stale-appscommand.