Skip to content

Conversation

@GitHK
Copy link
Contributor

@GitHK GitHK commented Jul 16, 2025

What do these changes do?

When validating the NATRule ooil did not allow to use quadruple $ characters.
And env var was added to enable this behaviour.
When packaging ci-service-integration-library this one var is set allowing ooil to properly validate.

Related issue/s

How to test

Dev-ops

@GitHK GitHK self-assigned this Jul 16, 2025
@GitHK GitHK added this to the Engage milestone Jul 16, 2025
@codecov
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

Attention: Patch coverage is 78.84615% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (289f936) to head (0e21209).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8118      +/-   ##
==========================================
+ Coverage   88.33%   90.04%   +1.71%     
==========================================
  Files        1853     1487     -366     
  Lines       71079    61007   -10072     
  Branches     1182      488     -694     
==========================================
- Hits        62785    54933    -7852     
+ Misses       7941     5948    -1993     
+ Partials      353      126     -227     
Flag Coverage Δ
integrationtests 64.23% <ø> (+<0.01%) ⬆️
unittests 88.40% <78.84%> (+0.75%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 93.14% <78.84%> (-0.13%) ⬇️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.17% <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (+8.24%) ⬆️
agent 93.81% <ø> (ø)
api_server 93.02% <ø> (ø)
autoscaling 95.88% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.81% <ø> (-0.34%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 76.32% <ø> (+0.18%) ⬆️
director_v2 91.05% <ø> (+0.04%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.06% <ø> (ø)
efs_guardian 89.76% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.60% <ø> (ø)
resource_usage_tracker 92.65% <ø> (+0.42%) ⬆️
storage 86.39% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 88.51% <ø> (-0.02%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 289f936...0e21209. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GitHK GitHK added a:ooil integration-library or ooil bug buggy, it does not work as expected t:maintenance Some planned maintenance work labels Jul 17, 2025
@GitHK GitHK marked this pull request as ready for review July 17, 2025 13:37
@GitHK GitHK requested a review from mguidon July 17, 2025 13:38
@GitHK GitHK requested a review from Copilot July 17, 2025 14:07

This comment was marked as outdated.

Andrei Neagu added 2 commits July 17, 2025 16:10
@GitHK GitHK requested a review from Copilot July 17, 2025 14:12
Copy link
Contributor

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 enhances the ooil validation to support quadruple dollar ($$$$) characters used by OsparcVariableIdentifier patterns. The main change introduces environment variable-controlled behavior that allows switching between platform-level validation (supporting 1-2 dollars) and ooil-level validation (supporting 1-4 dollars) for OSPARC variable identifiers.

Key changes:

  • Refactored OsparcVariableIdentifier to use a discriminated union pattern with environment variable control
  • Updated OpenAPI schema to support both 1-2 and 1-4 dollar patterns through oneOf schemas
  • Added test configuration to enable the new ooil behavior in service integration tests

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
services/director-v2/openapi.json Updated JSON schema patterns to support both 1-2 and 1-4 dollar variable identifiers using oneOf structures
packages/models-library/src/models_library/osparc_variable_identifier.py Refactored to use discriminated union with platform and ooil variants, added environment variable control
packages/models-library/src/models_library/utils/types.py Added utility function to extract types from annotated unions
packages/models-library/src/models_library/service_settings_nat_rule.py Updated isinstance checks to work with new discriminated union structure
packages/models-library/tests/test_service_settings_nat_rule.py Updated test assertions to use new type extraction utility
packages/service-integration/tests/test_osparc_image_specs.py Added environment variable setup to enable ooil behavior in tests
packages/service-integration/tests/data/runtime.yml Updated test data to include quadruple dollar variable examples
packages/service-integration/src/service_integration/init.py Minor docstring formatting change

@GitHK GitHK added the 🤖-automerge marks PR as ready to be merged for Mergify label Jul 18, 2025
@GitHK
Copy link
Contributor Author

GitHK commented Jul 18, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2025

queue

🛑 The pull request has been removed from the queue default

The following conditions don't match anymore:

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = unit-tests
        • check-skipped = unit-tests
        • check-success = unit-tests

@sonarqubecloud
Copy link

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@GitHK
Copy link
Contributor Author

GitHK commented Jul 18, 2025

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2025

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request.

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at e805af6

@mergify mergify bot merged commit e805af6 into ITISFoundation:master Jul 18, 2025
199 of 205 checks passed
@GitHK GitHK deleted the pr-osparc-ooil-escape-additional-env-vars branch July 18, 2025 05:08
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:ooil integration-library or ooil bug buggy, it does not work as expected t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants