Skip to content

Comments

fix: add validation for system caches delete flags#629

Merged
lucasrod16 merged 2 commits intomainfrom
lucas-system-caches-delete-required-flags-validation
Dec 19, 2024
Merged

fix: add validation for system caches delete flags#629
lucasrod16 merged 2 commits intomainfrom
lucas-system-caches-delete-required-flags-validation

Conversation

@lucasrod16
Copy link
Contributor

@lucasrod16 lucasrod16 commented Dec 18, 2024

Intent

Fixes #628

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

Use click's built-in validation, required=True

Automated Tests

Added tests to the TestSystemCachesDelete class

Directions for Reviewers

Checklist

  • I have updated CHANGELOG.md to cover notable changes.
  • I have updated all related GitHub issues to reflect their current state.

@github-actions
Copy link

github-actions bot commented Dec 18, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
4825 3575 74% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
rsconnect/api.py 71% 🟢
rsconnect/main.py 62% 🟢
TOTAL 66% 🟢

updated for commit: cb76491 by action🐍

@lucasrod16 lucasrod16 force-pushed the lucas-system-caches-delete-required-flags-validation branch 2 times, most recently from 3086e4e to e55e620 Compare December 18, 2024 23:59
Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>
@lucasrod16 lucasrod16 force-pushed the lucas-system-caches-delete-required-flags-validation branch from e55e620 to 955921e Compare December 19, 2024 00:04
Copy link
Collaborator

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @toph-allen - do you recall why we did not mark the language and version arguments as required?

The Connect API enforces the presence of language, version, and image name.

"--version",
"-V",
help="The version of the target cache.",
required=True,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system_caches_delete() function claims that language and version have Optional[..]. The delete_runtime_cache() function does the same. Should we update them to not have Optional[..] typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good catch. I removed the Optional typing

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>
@lucasrod16 lucasrod16 merged commit 801d8f0 into main Dec 19, 2024
14 checks passed
@lucasrod16 lucasrod16 deleted the lucas-system-caches-delete-required-flags-validation branch December 19, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: validate required flags for system caches delete command

3 participants