Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Better error messages when using unsupported compose file syntax #651

Merged

Conversation

jcsirot
Copy link
Contributor

@jcsirot jcsirot commented Sep 30, 2019

- What I did
Docker App prints a more specific error messages when the template uses one of these compose file syntax

  • ${VARIABLE:-default}
  • ${VARIABLE-default}
  • ${VARIABLE:?err}
  • ${VARIABLE?err}

- A picture of a cute animal (not mandatory but encouraged)
Hawaiian_bobtail_squid04 img_assist_custom

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Sounds good 👍

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #651 into master will decrease coverage by 0.04%.
The diff coverage is 87.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
- Coverage   72.19%   72.14%   -0.05%     
==========================================
  Files          51       52       +1     
  Lines        2658     2675      +17     
==========================================
+ Hits         1919     1930      +11     
- Misses        490      497       +7     
+ Partials      249      248       -1
Impacted Files Coverage Δ
internal/commands/root.go 72.11% <0%> (ø) ⬆️
internal/store/bundle.go 71% <100%> (ø) ⬆️
internal/commands/image/list.go 81.81% <100%> (-1.79%) ⬇️
internal/yaml/yaml.go 80% <100%> (ø) ⬆️
internal/commands/image/command.go 100% <100%> (ø) ⬆️
internal/commands/image/rm.go 61.29% <83.33%> (-8.48%) ⬇️
internal/commands/image/tag.go 88.23% <88.23%> (ø)
types/parameters/parameters.go 92.06% <0%> (-4.77%) ⬇️

Continue to review full report at Codecov.

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

render/render.go Outdated Show resolved Hide resolved
@jcsirot jcsirot force-pushed the composefile-syntax-improved-error-messages branch from ad9e6f6 to 9ad56e0 Compare October 1, 2019 07:46
@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "composefile-syntax-improved-error-messages" git@github.com:jcsirot/docker-app.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354106912
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@jcsirot jcsirot force-pushed the composefile-syntax-improved-error-messages branch from a32ff90 to 4e48f0f Compare October 1, 2019 09:10
@silvin-lubecki
Copy link
Contributor

@jcsirot can you squash?

@jcsirot jcsirot force-pushed the composefile-syntax-improved-error-messages branch from 4e48f0f to e1f8b4d Compare October 1, 2019 09:42
Replace errors.New by errors.Errorf since the linter complained about it
Fix typo in internal/commands/push.go

Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
@jcsirot jcsirot force-pushed the composefile-syntax-improved-error-messages branch from e1f8b4d to 79c5a88 Compare October 1, 2019 09:44
@jcsirot
Copy link
Contributor Author

jcsirot commented Oct 1, 2019

@silvin-lubecki done

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

@jcsirot I see that the errors are capitalised which I thought was against Go best practices.

@silvin-lubecki what do we do in the CLI?

render/render.go Outdated Show resolved Hide resolved
Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
@silvin-lubecki
Copy link
Contributor

@jcsirot I see that the errors are capitalised which I thought was against Go best practices.
@silvin-lubecki what do we do in the CLI?

You are right @chris-crone nice catch. Indeed the docker/cli error messages are not capitalized.

@jcsirot
Copy link
Contributor Author

jcsirot commented Oct 2, 2019

@chris-crone @silvin-lubecki Here the error message is composed of several sentences ending with ponctuation. So maybe we should keep it capitalised?

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants