-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 agent service followup fixes #3513
Conversation
Makefile
Outdated
@@ -242,7 +242,7 @@ CPU_COUNT = $(shell cat /proc/cpuinfo | grep processor | wc -l ) | |||
# -> filestash config at $(TMP_PATH_TO_FILESTASH_CONFIG) | |||
@$(shell \ | |||
export TMP_PATH_TO_FILESTASH_CONFIG="${TMP_PATH_TO_FILESTASH_CONFIG}" && \ | |||
docker-compose --env-file .env --file services/docker-compose-ops.yml --log-level=DEBUG config > $@ \ | |||
docker-compose --env-file .env --file services/docker-compose-ops.yml --log-level=DEBUG config | sed -E "s/cpus: ([0-9\\.]+)/cpus: '\\1'/" > $@ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand you intend to replace the cpus:
field to enforce single quotes '
right? If so the way you do it has quite some problems: Check your regex against these example https://regex101.com/r/a99Y5z/1
Moreover, if the problem is docker-compose
... please check the version
and the specs for it. Typically it is a problem with incompatibilities between the version of docker-compose
(i.e. the executable that runs config
) and the version of the compose specs file.
I would prefer solving this by aligning versions than patching the docker-compose
in the makefile with sed
. Notice how messy would it be if we have to do this with other fields. ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out by the post above, it's an issue with docker-compose
that is not getting fixed. Since It is currently frozen at version 1.29.2
. We have not upgraded to version 2.x.x which I'm not sure that if fixes this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so then I propose that you add a comment in this change so that we can follow up when we do the upgrade of docker-compose.
IMO patching the docker-compose this way should only be done in exceptional circumstances (this could be one of them!).
Regarding the regex... It realize now that I do not understand how really sed
works because
echo cpus: "4.3" | sed -E "s/cpus: ([0-9\\.]+)/cpus: '\\1'/"
cpus: '4.3'
echo "cpus: '4.3'" | sed -E "s/cpus: ([0-9\\.]+)/cpus: '\\1'/"
cpus: '4.3'
despite the fact that the regex clearly misses spaces \w
and the quotes as shown in https://regex101.com/r/a99Y5z/1
Please retry analysis of this Pull-Request directly on SonarCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please capture the env-vars noted in #3465 in the docker-compose.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment in this change with info about the issue so that we can follow up when we do the upgrade of docker-compose
thx
…core-forked into pr-osparc-agent-missing
Codecov Report
@@ Coverage Diff @@
## master #3513 +/- ##
========================================
- Coverage 82.6% 82.4% -0.3%
========================================
Files 852 852
Lines 35978 35978
Branches 759 759
========================================
- Hits 29752 29679 -73
- Misses 6029 6102 +73
Partials 197 197
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
What do these changes do?
This is a followup to #3465
Bonus: editing non service files will trigger CI jobs
Related issue/s
How to test
Checklist