Skip to content

Conversation

@GitHK
Copy link
Contributor

@GitHK GitHK commented Jul 9, 2025

What do these changes do?

With the latest versions of docker and docker compose, it is no longer possible to use $$ escaping in the image labels. The $$$$ is now required.
When the images are built, the $$$$ is converted to a single $ on the labels of the images.

Related issue/s

How to test

Dev-ops

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

codecov bot commented Jul 9, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.80%. Comparing base (e1d722b) to head (8e3c0ef).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8085      +/-   ##
==========================================
+ Coverage   88.16%   89.80%   +1.63%     
==========================================
  Files        1816     1281     -535     
  Lines       69980    54438   -15542     
  Branches     1264      255    -1009     
==========================================
- Hits        61699    48888   -12811     
+ Misses       7914     5470    -2444     
+ Partials      367       80     -287     
Flag Coverage Δ
integrationtests 64.25% <ø> (-0.03%) ⬇️
unittests 87.97% <85.71%> (+1.23%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.17% <85.71%> (+0.24%) ⬆️
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (-0.06%) ⬇️
agent 96.29% <ø> (ø)
api_server 92.83% <ø> (ø)
autoscaling 95.88% <ø> (ø)
catalog 92.58% <ø> (∅)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.57% <ø> (-0.79%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 76.86% <ø> (ø)
director_v2 91.04% <ø> (+0.02%) ⬆️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.60% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 92.52% <ø> (+0.05%) ⬆️
storage 86.60% <ø> (-0.21%) ⬇️
webclient ∅ <ø> (∅)
webserver 88.60% <ø> (+<0.01%) ⬆️

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 e1d722b...8e3c0ef. 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 changed the title ✨ ooil allows to escape legacy formation in configuration files ✨ allows ooil to escape legacy formation in y*ml files inside .osparc folder Jul 9, 2025
@GitHK GitHK requested a review from Copilot July 9, 2025 10:38
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 adds a new CLI command to escape legacy $${ sequences in YAML files under the .osparc config directory and provides corresponding tests.

  • Implement escape_dollar_brace and a legacy-escape command to replace $$\{ with $$$$\{ in .y*ml files.
  • Add unit tests for the escaping function and end-to-end tests for the new CLI command.
  • Update the CLI entrypoint to register the new legacy-escape command.

Reviewed Changes

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

File Description
packages/service-integration/src/service_integration/cli/_escaping.py Add escape logic and legacy_escape CLI implementation
packages/service-integration/src/service_integration/cli/init.py Register legacy-escape command
packages/service-integration/tests/test_cli__escaping.py Add parameterized tests for escape_dollar_brace
packages/service-integration/tests/test_cli.py Add tests for the legacy-escape CLI and error helper
Comments suppressed due to low confidence (1)

packages/service-integration/tests/test_cli.py:24

  • [nitpick] The os.EX_OK constant may not be defined on all platforms (e.g., non-Unix). For cross-platform tests, consider asserting against literal 0 or guarding availability.
    assert result.exit_code == os.EX_OK, _format_cli_error(result)

@GitHK GitHK marked this pull request as ready for review July 9, 2025 10:51
@GitHK GitHK requested review from pcrespov and sanderegg as code owners July 9, 2025 10:51
@GitHK GitHK changed the title ✨ allows ooil to escape legacy formation in y*ml files inside .osparc folder ✨ allows ooil to escape legacy format in y*ml files inside .osparc folder Jul 9, 2025
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

can you please document where this change comes from?

  • I searched a bit and found there was a temporary issue with some versions of compose
  • reading this PR I don't learn anything about the why, how, of this "change" if it's indeed a "change" and not a bug.

@GitHK
Copy link
Contributor Author

GitHK commented Jul 9, 2025

can you please document where this change comes from?

  • I searched a bit and found there was a temporary issue with some versions of compose
  • reading this PR I don't learn anything about the why, how, of this "change" if it's indeed a "change" and not a bug.

I've updated the only reference I found about this $ escaping in docker compose. But somehow if you use docker==28.1.1 this is not an issue. When you upgrade to docker==28.3.1 it is an issue.

@GitHK GitHK requested a review from sanderegg July 9, 2025 11:46
@GitHK GitHK requested a review from sanderegg July 9, 2025 12:04
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

will not block it, but I think this would need a bit more thorough search than just making it work. what happens if they fix it? will it break again?

@GitHK
Copy link
Contributor Author

GitHK commented Jul 10, 2025

will not block it, but I think this would need a bit more thorough search than just making it work. what happens if they fix it? will it break again?

I'm changing it so that it can easily be revertible with an option in the publisher pipeline.

@GitHK
Copy link
Contributor Author

GitHK commented Jul 10, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 415442c

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

@mergify mergify bot merged commit 415442c into ITISFoundation:master Jul 10, 2025
97 checks passed
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants