Skip to content
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

Fix required env vars ignored except the last one #242

Conversation

rimelek
Copy link
Contributor

@rimelek rimelek commented Mar 20, 2022

During the variable substitution, if a missing variable was followed
by an existing one, the empty "err" variable overwrote the previous
non-empty variable.

Unlike with Docker Compose v1, the first found error did not guarantee
that an actual error would be thrown.

Example which would have run before the fix even though var1 is not defined:

services:
  bash:
    image: bash:5.0.18-alpine3.15
    environment:
      var12: "_ ${var1:?Error1} _ ${var2:?Error2} _ "
    command:
      - env
# var2=1 docker compose up

This change also means that, if multiple variables are missing in one string, the first one will be reported by Docker Compose

Signed-off-by: Ákos Takács takacs.akos@it-sziget.hu

@rimelek rimelek requested a review from ndeloof as a code owner Mar 20, 2022
@@ -98,13 +102,19 @@ func SubstituteWith(template string, mapping Mapping, pattern *regexp.Regexp, su
)
value, applied, err = f(substitution, mapping)
if err != nil {
if errReturn == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be probably nicer to adopt https://github.com/hashicorp/go-multierror or comparable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. My first attempt was to handle every error message, but I didn't want to make such a "big" change, since I thought handling only one was intentional. That would be a good feature though.

When I wanted to handle multiple error messages, I used a simple slice to store the messages, but I realized I would have had to change the return value as well which could breake something else, so I returned only the first one, then I changed it to the current version.

Is the library "go-multierror" considered stable and maintained? The last change was a year ago. Of course, if something is perfect, then no need to change it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

as long as the signature isn't changed and we return error I guess it's fine, and we can consider this "implementation detail" we could later change if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be a new feature though rather than a fix? With the change I made, docker compose would work as it always worked in v1, reporting the first error. As of v2, it never worked correctly. I tried with 2.0.1 on Mac getting the same behavior. When I faced this error first, I found similar issues (docker/compose#8510) which I could not reproduce, but that was either solved since then or it happens in a specific situation which we could not find yet. I have more ideas to improve the test cases (we can discuss it somewhere else I guess) which I didn't want to implement yet either, because that would be an other change that you may or may not want to accept, but I could not add all the test cases I wanted to, since that is not possible yet without changing an other part of the code. It is possible that improving the test cases would solve other issues as well that we could not reproduce.

I am actually working on an example project on GitHub to show how PHP Composer can be used with Docker Compose without installing composer in the aplication container and somehow I always run into bugs. One was fixed in Docker Compose 2.3.1, then I ran into this error handling issue, so I thought I try to find and fix if I can instead of suggesting to my audience to use Docker Compose v1.

Your suggestion is a good one, I lke it, but I would rather not introduce a new dependency at this point. Maybe it can be solved without any dependency. For example by concatenating the error messages and returning a new error with that message, but I somehow don't think it can be decided so quickly.

If you have any suggestion what I should add to the pull request to fix parsing required variables, I would happily learn and add it, or if you have a completely different idea to fix the original issue without merging my change, that's fine to me :), but I think you should focus on the stability more than implementing new features like this, however, it would be a good improvement in the future.

Please, let me now, if I can help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you want to return the first error encountered, then you can just break execution by return nil, err and don't need to manage this errReturn variable

Copy link
Contributor Author

@rimelek rimelek Mar 30, 2022

Choose a reason for hiding this comment

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

Hi @ndeloof! Is there anything else I can do to get it fixed? :) Should I open an issue instead or do you think you can approve the pull request when you have more time to look into the code I changed? I really open to any suggestion that results a cleaner and more acceptable code while still focuses on fixing the bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for using multierror here.
Also adding a test with multiple variables failing so we could cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ulyssessouza yes, the tests need to be improved as well. It would have been my next goal, since this is why this error was not discovered before. I just added one test with two variables to validate what I changed.

As I wrote before, I totaly agree with that multierror feature, but in my mind that is a feature, which is very much required but every feature can introduce new bugs as well. When I committed this change, I could easily try it with Docker Compose and worked. I gave a presentation on a local Docker Community meetup about helping the community by answering questions on the forum, fixing small bugs and reporting issues and I promised to talk about it in an other video in details but by that time, I was not able to test the bugfix with Docker Compose because of new changes so I gave up.

Since I will not try to add new features in a bugfix in case of a bug which have been there for almost a year (I don't know if it is still there, because I didn't have time to check), I leave it to you how you think it should be fixed. I would just like to recommend and also ask that you try to fix the bug separately and release a patch version before implementing new changes so you can be sure that you will have a working version without any unexpected side effect.

Thank you for your work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So we can add the multierror in a followup PR.

Just a rebase and that would be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry, I forgot about this PR. Now I rebased the PR on the current master, but I had to resolve merge conflicts. I hope I did it right. Since you introduced a new variable "outerErr" I renamed my variable from "errReturn" to "returnErr" to keep the code consistent. Since the condition with the variable "err" was not returned, I did not add my lines there again.

In the meantime I also tried some multierror solution in one of my projects based on this post: https://medium.com/a-journey-with-go/go-multiple-errors-management-a67477628cf1

Eventually I decided not to use those because it wasn't clear to me how they were more than using "append" or concatenenating strings and it would not have solved my problem. I am sure you can do it right, so I can't wait to see the result.

Thank you and sorry for the delay

During the variable substitution, if a missing variable was followed
by an existing one, the empty "outerErr" variable overwrote the previous
non-empty variable.

Unlike with Docker Compose v1, the first found error did not guarantee
that an actual error would be thrown.

Example which would have run before the fix even though var1 is not defined:

```yaml
services:
  bash:
    image: bash:5.0.18-alpine3.15
    environment:
      var12: "_ ${var1:?Error1} _ ${var2:?Error2} _ "
    command:
      - env
```

```bash
```

This change also means that, if multiple variables are missing in one string, the first one will be reported by Docker Compose

Signed-off-by: Ákos Takács <takacs.akos@it-sziget.hu>
@rimelek rimelek force-pushed the fix-required-envvars-ignored-except-last branch from bf8a55f to 8ce8973 Compare Aug 31, 2022
@ulyssessouza ulyssessouza self-requested a review Sep 7, 2022
@ulyssessouza ulyssessouza merged commit ccdcc95 into compose-spec:master Sep 7, 2022
11 checks passed
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.

None yet

3 participants