Skip to content

fix(cssc): 31250186 address partial comments in public repo PR 20250416#47

Merged
cegraybl merged 8 commits into
feature/cssc_extfrom
cegraybl/pr_comments_2
Apr 16, 2025
Merged

fix(cssc): 31250186 address partial comments in public repo PR 20250416#47
cegraybl merged 8 commits into
feature/cssc_extfrom
cegraybl/pr_comments_2

Conversation

@cegraybl
Copy link
Copy Markdown

Second wave of PR comments for Azure#8530
Fix remaining open comments on public PR until 04/16/25
Test, style and linter are clean

@cegraybl cegraybl requested a review from Copilot April 16, 2025 16:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses final comments on public repo PR 20250416, improving error messaging, logging, and validation behavior within the continuous patch functionality. Key changes include:

  • Expanded test cases for timespan formats and enhanced validation methods.
  • Updates to error messaging and logging for task operations and ACR token retrieval.
  • Refactoring in utility and validator modules to improve clarity and maintainability.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/acrcssc/azext_acrcssc/tests/latest/test_validators.py Added extra test cases for timespan validation and updated patch decorators.
src/acrcssc/azext_acrcssc/tests/latest/test_helper_taskoperations.py Removed redundant mocks and simplified test setups.
src/acrcssc/azext_acrcssc/helper/_workflow_status.py Replaced wait() with a polling loop to update progress during thread execution.
src/acrcssc/azext_acrcssc/helper/_utility.py Updated regex for timespan parsing and improved error recommendations.
src/acrcssc/azext_acrcssc/helper/_taskoperations.py Refactored task deletions and improved error logging for task operations.
src/acrcssc/azext_acrcssc/helper/_ociartifactoperations.py Enhanced error logging and exception handling for ACR token retrieval and config validation.
src/acrcssc/azext_acrcssc/helper/_deployment.py Improved logging messages and string formatting in deployment routines.
src/acrcssc/azext_acrcssc/helper/_constants.py Added constants for JSON error messages and removed unused entries.
src/acrcssc/azext_acrcssc/_validators.py Refactored config and schedule validations; updated error handling logic.

Comment thread src/acrcssc/azext_acrcssc/_validators.py Outdated
@cegraybl cegraybl requested a review from Copilot April 16, 2025 17:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses outstanding review comments by updating test cases, improving error messages, and refining exception handling and logging across multiple modules. Key changes include:

  • Expanded test cases in validators to cover additional invalid schedule strings.
  • Updated JSON parsing and validation logic with clearer error messages.
  • Refactored logging and task cancellation flows to improve debugging and robustness.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/acrcssc/azext_acrcssc/tests/latest/test_validators.py Expanded invalid schedule test cases and updated JSON load method usage.
src/acrcssc/azext_acrcssc/tests/latest/test_helper_taskoperations.py Removed unused patch for task client and adjusted mocks accordingly.
src/acrcssc/azext_acrcssc/helper/_workflow_status.py Replaced concurrent.futures.wait with a polling loop to update progress.
src/acrcssc/azext_acrcssc/helper/_utility.py Updated regex for timespan parsing and enhanced error raising with recomendation.
src/acrcssc/azext_acrcssc/helper/_taskoperations.py Improved logging details and refactored task deletion to use actual task names.
src/acrcssc/azext_acrcssc/helper/_ociartifactoperations.py Enhanced ACR token retrieval error handling and JSON validation in config.
src/acrcssc/azext_acrcssc/helper/_deployment.py Improved logging format and cleaned up template deployment message formats.
src/acrcssc/azext_acrcssc/helper/_constants.py Removed unused constants and added new error message constants.
src/acrcssc/azext_acrcssc/_validators.py Refactored config validation to use the ContinuousPatchConfig instance.
Comments suppressed due to low confidence (1)

src/acrcssc/azext_acrcssc/_validators.py:58

  • The type check was changed from dict to ContinuousPatchConfig. Confirm that all downstream consumers of the config support the new ContinuousPatchConfig instance format rather than a plain dictionary.
if not isinstance(config, ContinuousPatchConfig):

@cegraybl cegraybl merged commit f48d949 into feature/cssc_ext Apr 16, 2025
@cegraybl cegraybl deleted the cegraybl/pr_comments_2 branch April 23, 2025 17:11
cegraybl added a commit that referenced this pull request Apr 23, 2025
…16 (#47)

Second wave of PR comments for
Azure#8530
Fix remaining open comments on public PR until 04/16/25
Test, style and linter are clean
cegraybl added a commit that referenced this pull request Apr 24, 2025
…16 (#47)

Second wave of PR comments for
Azure#8530
Fix remaining open comments on public PR until 04/16/25
Test, style and linter are clean
mabelegba pushed a commit that referenced this pull request Nov 25, 2025
* Upstream Merge

* Merge from workload-orchestration : Added Example in Description (#34)

* Merge from workload-orchestration : Fixed Example

* Fixed Example for SolutionTemplateVersion

* Made Changes for command-change CI fix'

* Reset Version 1.0.0b1

* Added Bulk and Diagnostics Back (#35)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added Bulk Solution Example (#36)

* Added Bulk and Diagnostics Back

* Changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Fixed Solution Template Linter Issue

* Added Integration Tests Framework for WorkloadOrchestration #37

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added ServiceName  (#38)

* Added Tests

* Made Changes on ServiceName

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added Full Target Solution Tests (#39)

* Added Tests

* Made Changes on ServiceName

* Nit Changes

* Made Changes for ContextLookup

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added E2E workflow Tests (#40)

* Added Tests

* Made Changes on ServiceName

* Nit Changes

* Made Changes for ContextLookup

* Added complete workflow

* Add Licence

* Added License

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added Readme (#41)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Bulk Deployemnent LRO changes (#42)

* made changes

* Added change

* Added Changes in Test

* Added CLi Changes

* Added Setup

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Added Changes For Backward Compitablity (#43)

* Added Changes For Backward Compitablity

* Made changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Made changes (#44)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Target Operations API refractoring  (#47)

* Changes I made

* Make changes

* Made chnages

* Make changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Stable API 2025-06-01 (#48)

* Made changes

* Made Changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Change in ID in Targets (#50)

* Made changes

* changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Remove Id and Name from Target Review  (#51)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* final bulk (#52)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Changes in ARG

* Added Linter Exception

* Added ITTests (#53)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Standardize CLI (#54)

* Made Changes

* Made Changes

* Made changes

* Added Change

* Made changes

* Made changes

* Made changes

* Made Changes

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Linter FIxes and Remove Preview

* Removed Resolved from CLI

* change in version

* made changes (#55)

Co-authored-by: Atharva Udapure <audapure@microsoft.com>

* Fixed history

---------

Co-authored-by: Atharva Udapure <audapure@microsoft.com>
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.

2 participants